From fe0eef478396cfed21143d1ede8f5e4592a64bcc Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 11:37:50 +0200 Subject: [PATCH 01/18] Add render/stream performance plans Co-Authored-By: Claude Opus 5 --- plans/001-lookup-array-fast-path.md | 269 ++++++++++++++++ plans/002-lazy-variable-lookup.md | 426 +++++++++++++++++++++++++ plans/003-bodynode-child-overhead.md | 445 +++++++++++++++++++++++++++ plans/README.md | 92 ++++++ 4 files changed, 1232 insertions(+) create mode 100644 plans/001-lookup-array-fast-path.md create mode 100644 plans/002-lazy-variable-lookup.md create mode 100644 plans/003-bodynode-child-overhead.md create mode 100644 plans/README.md diff --git a/plans/001-lookup-array-fast-path.md b/plans/001-lookup-array-fast-path.md new file mode 100644 index 0000000..3d21452 --- /dev/null +++ b/plans/001-lookup-array-fast-path.md @@ -0,0 +1,269 @@ +# Plan 001: Fast-path array scopes in `RenderContext::internalContextLookup()` + +> **Executor instructions**: Follow this plan step by step. Run every +> verification command and confirm the expected result before moving to the +> next step. If anything in the "STOP conditions" section occurs, stop and +> report — do not improvise. When done, update the status row for this plan +> in `plans/README.md`. +> +> **Drift check (run first)**: `git diff --stat 4a1bdd6..HEAD -- src/Render/RenderContext.php` +> If that file changed since this plan was written, compare the "Current state" +> excerpt against the live code before proceeding; on a mismatch, treat it as a +> STOP condition. + +## Status + +- **Priority**: P1 +- **Effort**: S +- **Risk**: LOW +- **Depends on**: none +- **Category**: perf +- **Planned at**: commit `4a1bdd6`, 2026-07-29 + +## Why this matters + +`internalContextLookup()` is the innermost function of variable resolution: it +runs **8989 times per render pass** of the theme benchmark suite. Today every +one of those calls enters a `try`/`catch` block and walks a `match (true)` +chain whose *first* arm tests `$scope instanceof Drop` — even though the +overwhelming majority of scopes are plain PHP arrays (the render scopes, the +context data, and the static variables are all arrays). Hoisting the array case +out of the `match` and above the `try` measured **−12.5% on render and −11.3% +on stream** — the single largest win found in the render phase, from one hunk. + +Semantics do not change: the array arm produces exactly the same value, and the +"key absent" case produces the same `MissingValue` the `default` arm produced. + +## Current state + +- `src/Render/RenderContext.php` — the render-time variable context. The method + to change is `internalContextLookup()` at lines 221–236. + +Exact code as it exists today at `src/Render/RenderContext.php:221-236`: + +```php + public function internalContextLookup(mixed $scope, int|string $key): mixed + { + try { + $value = match (true) { + $scope instanceof Drop => $scope->{$key}, + is_array($scope) && array_key_exists($key, $scope) => $scope[$key], + is_object($scope) && $this->objectHasProperty($scope, (string) $key) => $scope->{$key}, + is_object($scope) && $this->objectHasStaticProperty($scope, (string) $key) => $scope::$$key, + default => $this->missingValue, + }; + } catch (UndefinedDropMethodException) { + return $this->missingValue; + } + + return $this->normalizeValue($value); + } +``` + +Facts you need about the surrounding code: + +- `$this->missingValue` is a private, pre-allocated `MissingValue` instance + (declared at `src/Render/RenderContext.php:65`, constructed at line 102). + Callers detect "not found" with `$value instanceof MissingValue`. +- `normalizeValue()` (same file) resolves `Closure` and `MapsToLiquid` values + and must still be applied to whatever the array branch returns. +- An array scope can never throw `UndefinedDropMethodException` — that exception + comes from `Drop::__get()` (`src/Drop.php`) and from `objectHasProperty()` + reaching a magic getter. So moving the array case outside the `try` is safe. + +Repo conventions: PHP 8.2+, strict PHPStan level 9 over `src/`, formatting by +Laravel Pint (`pint.json`). Match the existing style in this file — early +returns and guard clauses are already used throughout (see `normalizeValue()` +and `findVariables()` in the same file). + +## Commands you will need + +| Purpose | Command | Expected on success | +|---------|---------|---------------------| +| Tests | `vendor/bin/pest` | `Tests: 800 passed` (or more), 0 failures | +| Static analysis | `vendor/bin/phpstan analyse --no-progress` | `[OK] No errors` | +| Formatting | `vendor/bin/pint --test` | exit 0 (no files need fixing) | +| Benchmark | see "Benchmark procedure" below | render/stream both improve | + +## Benchmark procedure + +Run this from the repo root, after the code change and before committing: + +```bash +vendor/bin/phpbench run \ + --filter='benchRender|benchStream' \ + --progress=none \ + --warmup=1 \ + --retry-threshold=5 \ + --report=aggregate \ + --output=json > build/plan-001.json + +php tools/phpbench-compare.php build/base.json build/plan-001.json +``` + +`build/base.json` is the committed baseline for this repo (captured at +`4a1bdd6`). The compare tool only reports on benchmark names present in both +files, so filtering to two subjects is expected and fine. + +**Expected**: both `LiquidBench::benchRender` and `LiquidBench::benchStream` +show a positive ops/s delta, roughly **+10% to +14%** each. Benchmark noise on +a developer laptop is ±2–3%; if you see a delta smaller than +5%, re-run once +before drawing a conclusion. A *negative* delta on either subject is a STOP +condition. + +Do not commit `build/plan-001.json` — `build/` output other than the committed +`base.json` is scratch. Check `git status` before committing. + +## Scope + +**In scope** (the only files you should modify): +- `src/Render/RenderContext.php` +- `plans/README.md` (status row only) + +**Out of scope** (do NOT touch, even though they look related): +- `src/Render/RenderContext.php`'s `objectHasProperty()` and + `objectHasStaticProperty()` — they are cold in this benchmark (0 calls) and + changing them adds risk for no measured gain. +- `normalizeValue()` — an early return for non-objects was measured at exactly + zero delta and was deliberately rejected. Do not add it. +- `src/Drop.php`, `src/Nodes/VariableLookup.php`, `src/Nodes/BodyNode.php` — + covered by other plans or explicitly rejected. +- `build/base.json` — the committed baseline. Never regenerate or overwrite it. + +## Git workflow + +- Work on branch `perf/render-stream-optimizations`. Create it from `main` if it + does not exist yet: `git switch -c perf/render-stream-optimizations`. If it + already exists (a previous plan created it), just switch to it. +- One commit for this plan. Message style in this repo is plain sentence case, + no conventional-commit prefixes (see `git log --oneline`: "Optimize parser and + renderer hot paths", "Skip partial ParseContext on cache hit, tidy tag + parsing"). Use: `Fast-path array scopes in context lookup`. +- Do NOT push and do NOT open a PR. + +## Steps + +### Step 1: Hoist the array case above the try block + +In `src/Render/RenderContext.php`, replace the body of +`internalContextLookup()` so that arrays are handled first, and the `match` +keeps only the remaining arms: + +```php + public function internalContextLookup(mixed $scope, int|string $key): mixed + { + if (is_array($scope)) { + if (! array_key_exists($key, $scope)) { + return $this->missingValue; + } + + return $this->normalizeValue($scope[$key]); + } + + try { + $value = match (true) { + $scope instanceof Drop => $scope->{$key}, + is_object($scope) && $this->objectHasProperty($scope, (string) $key) => $scope->{$key}, + is_object($scope) && $this->objectHasStaticProperty($scope, (string) $key) => $scope::$$key, + default => $this->missingValue, + }; + } catch (UndefinedDropMethodException) { + return $this->missingValue; + } + + return $this->normalizeValue($value); + } +``` + +Note the removed `is_array($scope) && array_key_exists($key, $scope)` arm — it +is now unreachable and must be deleted, not left in place. + +**Verify**: `vendor/bin/pest` → `Tests: 800 passed` (or more), 0 failures. + +### Step 2: Confirm static analysis and formatting + +**Verify**: +- `vendor/bin/phpstan analyse --no-progress` → `[OK] No errors` +- `vendor/bin/pint --test` → exit 0 + +If Pint reports the file needs formatting, run `vendor/bin/pint src/Render/RenderContext.php` +and re-run the tests. + +### Step 3: Benchmark + +Run the "Benchmark procedure" above. + +**Verify**: `php tools/phpbench-compare.php build/base.json build/plan-001.json` +→ positive ops/s delta on both `LiquidBench::benchRender` and +`LiquidBench::benchStream`, expected around +10% to +14%. + +Record the two delta percentages — you will report them back. + +### Step 4: Commit + +```bash +git status # confirm only src/Render/RenderContext.php and plans/README.md changed +git add src/Render/RenderContext.php plans/README.md +git commit -m "Fast-path array scopes in context lookup" +``` + +**Verify**: `git show --stat HEAD` → exactly two files changed; `git status` → +clean except untracked `build/plan-001.json`. + +## Test plan + +No new tests are required: this is a behaviour-preserving refactor of an +internal method that is already covered end to end. The existing coverage that +exercises this path: + +- `tests/Integration/ContextTest.php` — scope resolution, nested scopes, + static data and registers. +- `tests/Integration/VariableTest.php` and `tests/Integration/DropTest.php` — + array scopes, drop scopes, and missing-variable behaviour. +- `tests/Integration/SelfDropTest.php` — the `self` fallback path, which + depends on `MissingValue` being returned for absent keys. + +If any of these fail, the array fast path is not behaviour-preserving — +that is a STOP condition, not something to patch around. + +**Verification**: `vendor/bin/pest` → all pass, same count as before the change. + +## Done criteria + +ALL must hold: + +- [ ] `vendor/bin/pest` exits 0 with the same test count as before the change +- [ ] `vendor/bin/phpstan analyse --no-progress` reports `[OK] No errors` +- [ ] `vendor/bin/pint --test` exits 0 +- [ ] `grep -n 'is_array($scope) && array_key_exists' src/Render/RenderContext.php` returns no matches +- [ ] `php tools/phpbench-compare.php build/base.json build/plan-001.json` shows a positive delta on both `benchRender` and `benchStream` +- [ ] `git show --stat HEAD` lists only `src/Render/RenderContext.php` and `plans/README.md` +- [ ] `plans/README.md` status row for 001 updated to DONE + +## STOP conditions + +Stop and report back (do not improvise) if: + +- The code at `src/Render/RenderContext.php:221-236` does not match the + "Current state" excerpt. +- Any test fails after the change. Do not adjust tests to fit — a failure here + means the fast path changed behaviour, which it must not. +- The benchmark shows a *negative* delta on either subject across two runs. +- You find yourself needing to modify any file outside the in-scope list. +- `build/base.json` does not exist or `tools/phpbench-compare.php` exits + non-zero for a reason other than a regression threshold. + +## Maintenance notes + +- The array branch bypasses the `try`/`catch`. That is only correct while array + access cannot throw `UndefinedDropMethodException`. If a future change makes + `$scope` an `ArrayAccess` object routed through this branch, the guard must be + revisited — `is_array()` deliberately excludes `ArrayAccess`, keep it that way. +- The branch must keep calling `normalizeValue()`; dropping it would silently + break `Closure` and `MapsToLiquid` values stored directly in a scope. A + reviewer should check for exactly that. +- Deferred out of this plan: the `objectHasProperty()` path still calls + `get_object_vars()` (allocating an array of every public property) on each + probe of a plain-object scope. It measured 0 calls in the theme benchmark, so + it was left alone; it would matter for an application that puts plain PHP + objects into the render context. diff --git a/plans/002-lazy-variable-lookup.md b/plans/002-lazy-variable-lookup.md new file mode 100644 index 0000000..0154c8a --- /dev/null +++ b/plans/002-lazy-variable-lookup.md @@ -0,0 +1,426 @@ +# Plan 002: Stop scanning every scope — add a lazy `RenderContext::iterateVariables()` + +> **Executor instructions**: Follow this plan step by step. Run every +> verification command and confirm the expected result before moving to the +> next step. If anything in the "STOP conditions" section occurs, stop and +> report — do not improvise. When done, update the status row for this plan +> in `plans/README.md`. +> +> **Drift check (run first)**: `git diff --stat 4a1bdd6..HEAD -- src/Render/RenderContext.php src/Nodes/VariableLookup.php` +> Plan 001 (`plans/001-lookup-array-fast-path.md`) also edits +> `src/Render/RenderContext.php`, in `internalContextLookup()`. A diff limited +> to that method is expected. Any other change to the two methods quoted under +> "Current state" is a STOP condition. + +## Status + +- **Priority**: P1 +- **Effort**: S +- **Risk**: LOW +- **Depends on**: plans/001-lookup-array-fast-path.md (ordering only — not a hard dependency) +- **Category**: perf +- **Planned at**: commit `4a1bdd6`, 2026-07-29 + +## Why this matters + +`findVariables()` resolves a bare variable name against every scope. Today it +*always* probes all of them — every render scope, then the context data, then +the static variables — collects every match into an array, and returns it. +Instrumenting one render pass of the theme benchmark: **1862 calls performing +7394 scope probes**, while nearly every caller consumes only the first match. + +The extra matches are not dead weight in general: `VariableLookup::evaluate()` +falls back to the next candidate when a *lookup path* (`a.b.c`) misses on the +nearest one. But that fallback is rare, and for a bare `{{ foo }}` — which has +no lookups at all — the later probes can never be used. + +Making the scan lazy (a `Generator` that yields matches as it finds them) lets +both callers stop at the first usable value while keeping the fallback semantics +exactly intact. Measured on top of plan 001: **−7.6% render, −6.9% stream**. + +`findVariables()` stays in place with its current signature and behaviour, so +this is not a breaking change for anyone calling it from a custom tag or drop. + +## Current state + +Two files change. + +### `src/Render/RenderContext.php` — the scope scanner + +Exact code today at `src/Render/RenderContext.php:180-214`: + +```php + public function findVariables(string $key): array + { + $variables = []; + + // Check the variable in all scopes + env data + static variables + $scopeCount = count($this->scopes); + for ($index = 0; $index < $scopeCount + 2; $index++) { + $scope = match (true) { + $index < $scopeCount => $this->scopes[$index], + $index === $scopeCount => $this->data, + default => $this->sharedState->staticVariables, + }; + + $value = $this->internalContextLookup($scope, $key); + + if (! $value instanceof MissingValue) { + $variables[] = $value; + } + } + + // Inject the implicit self drop only when no value (including explicit null) was found. + // An explicit `self = nil` leaves [null] in $variables, so the fallback is skipped, + // correctly distinguishing defined-null from undefined. + if ($variables === [] && $key === 'self') { + return [$this->getSelfDrop()]; + } + + foreach ($variables as $variable) { + if ($variable instanceof IsContextAware) { + $variable->setContext($this); + } + } + + return $variables; + } +``` + +Three behaviours in there are load-bearing and must survive: + +1. **Scan order**: innermost scope first (`$this->scopes[0]` … `[n]`), then + `$this->data`, then `$this->sharedState->staticVariables`. +2. **The `self` fallback**: the implicit `SelfDrop` is injected *only* when + nothing at all was found. An explicit `self = nil` yields `[null]`, which is + not empty, so the fallback is correctly skipped. Preserve that distinction — + track "did we find anything" as a flag; do not test the yielded value for + `null`. +3. **`setContext()`**: every returned value implementing `IsContextAware` gets + the context injected before the caller sees it. + +### `src/Nodes/VariableLookup.php` — the main consumer + +Exact code today at `src/Nodes/VariableLookup.php:89-101` (the head of +`evaluate()`): + +```php + public function evaluate(RenderContext $context): mixed + { + $name = $context->evaluate($this->name); + assert(is_string($name)); + $variables = $context->findVariables($name); + + if ($this->lookups === []) { + if ($context->options->strictVariables && $variables === []) { + return new UndefinedVariable($this->toString()); + } + + return $variables[0] ?? null; + } +``` + +and the rest of `evaluate()` (lines 103–135), which iterates the candidates and +uses `continue 2` to fall through to the next one when a lookup misses: + +```php + foreach ($variables as $object) { + $object = $context->evaluate($object); + + if ($object instanceof \Generator) { + $object = iterator_to_array($object, preserve_keys: false); + } + + foreach ($this->lookups as $i => $lookup) { + // ... lookup resolution ... + + if ($nextObject instanceof MissingValue) { + continue 2; + } + // ... + } + + return $object; + } + + return $context->options->strictVariables ? new UndefinedVariable($this->toString()) : null; +``` + +That `foreach` works unchanged over a `Generator` — this is why the change is +small. + +### Other callers + +`src/Drops/SelfDrop.php:20` and `:27` also call `findVariables()`. They are +**not** in scope: `findVariables()` keeps working exactly as before. + +### Conventions + +PHP 8.2+, PHPStan level 9 over `src/` (so the new method needs a +`@return \Generator` docblock — the bare `\Generator` return type is not +enough at level 9), Laravel Pint formatting. `\Generator` is already used as a +return type across this codebase without a `use` import — see +`src/Nodes/BodyNode.php::stream()` and `src/Nodes/Variable.php::stream()`. +Match that: write `\Generator`, do not add an import. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---------|---------|---------------------| +| Tests | `vendor/bin/pest` | `Tests: 800 passed` (or more), 0 failures | +| Static analysis | `vendor/bin/phpstan analyse --no-progress` | `[OK] No errors` | +| Formatting | `vendor/bin/pint --test` | exit 0 | +| Benchmark | see "Benchmark procedure" | render/stream both improve | + +## Benchmark procedure + +```bash +vendor/bin/phpbench run \ + --filter='benchRender|benchStream' \ + --progress=none \ + --warmup=1 \ + --retry-threshold=5 \ + --report=aggregate \ + --output=json > build/plan-002.json + +php tools/phpbench-compare.php build/base.json build/plan-002.json +``` + +`build/base.json` is the committed baseline at `4a1bdd6` — it is the baseline +for *all* plans, so this comparison shows the **cumulative** gain of plan 001 + +plan 002, expected around **+18% to +22%** on both subjects. To isolate this +plan's own contribution, compare against plan 001's result instead: +`php tools/phpbench-compare.php build/plan-001.json build/plan-002.json` +→ expected around **+6% to +9%** on both subjects. + +If `build/plan-001.json` does not exist (plan 001 was run by someone else, or in +a different working copy), skip the isolated comparison and report only the +cumulative one. + +Do not commit anything under `build/` other than the already-committed +`base.json`. + +## Scope + +**In scope** (the only files you should modify): +- `src/Render/RenderContext.php` +- `src/Nodes/VariableLookup.php` +- `tests/Integration/ContextTest.php` (add tests — see "Test plan") +- `plans/README.md` (status row only) + +**Out of scope** (do NOT touch, even though they look related): +- `src/Drops/SelfDrop.php` — it calls `findVariables()`, which keeps its exact + current signature and behaviour. Leave it alone. +- The `foreach ($variables as $object)` lookup-fallback loop in + `VariableLookup::evaluate()` (lines 103 onward) — it already works over a + `Generator` unchanged. Do not restructure it. +- `src/Render/RenderContext.php::internalContextLookup()` — that is plan 001. +- `build/base.json`. + +## Git workflow + +- Work on branch `perf/render-stream-optimizations` (created by plan 001; create + it from `main` with `git switch -c perf/render-stream-optimizations` if it + does not exist). +- One commit for this plan. Repo message style is plain sentence case, no + conventional-commit prefixes. Use: `Resolve variables lazily instead of scanning every scope`. +- Do NOT push and do NOT open a PR. + +## Steps + +### Step 1: Add `iterateVariables()` and re-express `findVariables()` on top of it + +In `src/Render/RenderContext.php`, replace the whole `findVariables()` method +with these two methods: + +```php + public function findVariables(string $key): array + { + return iterator_to_array($this->iterateVariables($key), preserve_keys: false); + } + + /** + * @return \Generator + */ + public function iterateVariables(string $key): \Generator + { + $found = false; + + // Check the variable in all scopes + env data + static variables + $scopeCount = count($this->scopes); + for ($index = 0; $index < $scopeCount + 2; $index++) { + $scope = match (true) { + $index < $scopeCount => $this->scopes[$index], + $index === $scopeCount => $this->data, + default => $this->sharedState->staticVariables, + }; + + $value = $this->internalContextLookup($scope, $key); + + if (! $value instanceof MissingValue) { + $found = true; + + if ($value instanceof IsContextAware) { + $value->setContext($this); + } + + yield $value; + } + } + + // Inject the implicit self drop only when no value (including explicit null) was found. + // An explicit `self = nil` yields null before this point, so $found is true and the + // fallback is skipped, correctly distinguishing defined-null from undefined. + if (! $found && $key === 'self') { + yield $this->getSelfDrop(); + } + } +``` + +Two differences from the original that are intentional: + +- `setContext()` now happens per value as it is yielded, rather than in a second + pass over the collected array. Consumers see an identical result. +- The `self` fallback is driven by the `$found` flag instead of + `$variables === []`, because a generator cannot inspect what it already + yielded. This preserves the defined-null-vs-undefined distinction. + +**Verify**: `vendor/bin/pest` → all pass. `findVariables()` is now a thin +wrapper, so every existing consumer must still be green at this point, before +any caller is switched over. + +### Step 2: Switch `VariableLookup::evaluate()` to the lazy path + +In `src/Nodes/VariableLookup.php`, change the head of `evaluate()` from the +"Current state" excerpt to: + +```php + public function evaluate(RenderContext $context): mixed + { + $name = $context->evaluate($this->name); + assert(is_string($name)); + $variables = $context->iterateVariables($name); + + if ($this->lookups === []) { + foreach ($variables as $variable) { + return $variable; + } + + return $context->options->strictVariables ? new UndefinedVariable($this->toString()) : null; + } +``` + +Leave everything from `foreach ($variables as $object) {` onward exactly as it +is — it consumes the generator correctly without modification. + +Why the `foreach`-then-`return` shape: it takes the first yielded value and +abandons the generator, so the remaining scopes are never probed. `null` is a +legitimate first value (an explicitly-null variable), which is why this cannot +be written as a null-coalescing expression. + +**Verify**: +- `vendor/bin/pest` → all pass, same count as before. +- `vendor/bin/phpstan analyse --no-progress` → `[OK] No errors`. +- `vendor/bin/pint --test` → exit 0. + +### Step 3: Add regression tests for the lazy path + +See "Test plan" below for the exact cases. + +**Verify**: `vendor/bin/pest --filter='lazy variable'` → the new tests pass. +Then `vendor/bin/pest` → full suite green with the new tests included. + +### Step 4: Benchmark + +Run the "Benchmark procedure" above. Record both delta percentages. + +**Verify**: positive ops/s delta on both `benchRender` and `benchStream` +versus `build/base.json`. + +### Step 5: Commit + +```bash +git status +git add src/Render/RenderContext.php src/Nodes/VariableLookup.php tests/Integration/ContextTest.php plans/README.md +git commit -m "Resolve variables lazily instead of scanning every scope" +``` + +**Verify**: `git show --stat HEAD` → exactly the four files above. + +## Test plan + +Add three tests to `tests/Integration/ContextTest.php`. Model them structurally +on the tests already in that file — Pest `test('...', function () { ... })` with +`expect(...)->toBe(...)`; read two neighbouring tests first and match their +setup style for building a `RenderContext`. + +The cases, each pinning one behaviour this change could plausibly break: + +1. **"lazy variable resolution returns the innermost scope"** — the same name + defined in an outer scope and in a pushed inner scope resolves to the inner + value. Guards scan order. +2. **"lazy variable resolution distinguishes defined-null from undefined"** — + a variable explicitly set to `null` resolves to `null` (not to a fallback, + and not to an `UndefinedVariable` under `strictVariables`), while a name that + was never set resolves to `null` normally and to `UndefinedVariable` under + `strictVariables`. Guards the `$found` flag. +3. **"lazy variable resolution falls back when a lookup path misses"** — with + `a` defined in two scopes, the inner one an array *without* key `b` and the + outer one an array *with* key `b`, `a.b` resolves to the outer value. Guards + the `continue 2` fallback that the generator must still support. + +Case 3 is the important one: it is the only reason `findVariables()` collected +multiple candidates in the first place, and it is the behaviour a naive +"return the first match" rewrite would silently destroy. If `tests/Integration/ContextTest.php` +already covers it, say so in your report and skip adding a duplicate. + +Also confirm the `self` fallback stays covered — `tests/Integration/SelfDropTest.php` +exercises it; it must pass untouched. + +**Verification**: `vendor/bin/pest` → all pass, including 2–3 new tests. + +## Done criteria + +ALL must hold: + +- [ ] `vendor/bin/pest` exits 0, with 2–3 more tests than before the change +- [ ] `vendor/bin/phpstan analyse --no-progress` reports `[OK] No errors` +- [ ] `vendor/bin/pint --test` exits 0 +- [ ] `grep -n 'findVariables' src/Nodes/VariableLookup.php` returns no matches +- [ ] `grep -n 'public function findVariables' src/Render/RenderContext.php` still returns a match (the method was kept for BC) +- [ ] `php tools/phpbench-compare.php build/base.json build/plan-002.json` shows a positive delta on both `benchRender` and `benchStream` +- [ ] `git show --stat HEAD` lists only the four in-scope files +- [ ] `plans/README.md` status row for 002 updated to DONE + +## STOP conditions + +Stop and report back (do not improvise) if: + +- The code at `src/Render/RenderContext.php:180-214` or + `src/Nodes/VariableLookup.php:89-101` does not match the "Current state" + excerpts (beyond plan 001's edit to `internalContextLookup()`). +- `tests/Integration/SelfDropTest.php` fails — that means the `self` fallback + distinction broke, which is the subtlest risk in this plan. +- Any existing test fails. Do not modify existing tests to make them pass. +- The benchmark shows a negative delta versus `build/base.json` on either + subject across two runs. +- You conclude the lookup-fallback loop needs restructuring to work with a + generator. It does not — if it seems to, something else is wrong. + +## Maintenance notes + +- `findVariables()` and `iterateVariables()` must not drift apart. If a future + change adds a scope source (a fourth place to look), it goes in + `iterateVariables()` only; `findVariables()` stays a one-line wrapper. +- The generator is consumed twice-shaped but never rewound. A caller that needs + the candidate list more than once must use `findVariables()`, not + `iterateVariables()` — generators are single-pass. A reviewer should check any + new consumer for that. +- `setContext()` is now called only on values that are actually reached. If some + code depended on the old eager behaviour — every candidate getting a context, + including ones never used — it would change. Nothing in this repo does; that + was verified by grepping the callers of `findVariables()` before writing this + plan. +- Deferred: `SelfDrop::__get()` and `__isset()` still call `findVariables()` + eagerly (`src/Drops/SelfDrop.php:20`, `:27`). They are cold in the theme + benchmark, so switching them was left out to keep this diff small. diff --git a/plans/003-bodynode-child-overhead.md b/plans/003-bodynode-child-overhead.md new file mode 100644 index 0000000..22f7e5b --- /dev/null +++ b/plans/003-bodynode-child-overhead.md @@ -0,0 +1,445 @@ +# Plan 003: Remove per-child overhead in the `BodyNode` render and stream loops + +> **Executor instructions**: Follow this plan step by step. Run every +> verification command and confirm the expected result before moving to the +> next step. If anything in the "STOP conditions" section occurs, stop and +> report — do not improvise. When done, update the status row for this plan +> in `plans/README.md`. +> +> **Drift check (run first)**: `git diff --stat 4a1bdd6..HEAD -- src/Nodes/BodyNode.php src/Render/RenderContext.php` +> `src/Nodes/BodyNode.php` must be unchanged since `4a1bdd6` — plans 001 and 002 +> do not touch it. `src/Render/RenderContext.php` will show plan 001's and plan +> 002's edits; that is expected. Any change to `hasInterrupt()` or to +> `BodyNode` is a STOP condition. + +## Status + +- **Priority**: P1 +- **Effort**: S +- **Risk**: LOW +- **Depends on**: plans/001, plans/002 (ordering only — not a hard dependency) +- **Category**: perf +- **Planned at**: commit `4a1bdd6`, 2026-07-29 + +## Why this matters + +`BodyNode` is the loop that walks every node of a template. One render pass of +the theme benchmark runs it 361 times over **4384 child nodes**, so anything +paid per child is paid thousands of times. Four things are paid per child today +and do not need to be: + +1. **`streamChild()` allocates a `Generator` for every non-streamable node.** + Text nodes and most tags are not `CanBeStreamed`, so streaming a typical + template allocates one generator object per node just to yield a single + string, plus the `yield from` delegation on top. +2. **`$node instanceof Tag` fires a method call on every tag node.** + `Tag::ensureTagIsEnabled()` (`src/Tag.php`) starts with + `if (! $this instanceof Disableable) { return; }` — so for the vast majority + of tags the call exists only to return immediately. Testing `Disableable` + at the call site skips it. +3. **`renderChild()` is an indirection with no purpose** — it has no overrides + anywhere in `src/`, `tests/`, or `performance/` (verified by grep) and does + nothing but forward to `$node->render($context)`. +4. **`hasInterrupt()` calls `count()`** on an array that is empty in the common + case, once per child, where a `!== []` comparison suffices. + +Measured as a bundle on top of plans 001 and 002: **−5.9% render, −9.4% stream**. +The four items were measured together, not individually — item 1 is +stream-only, items 2–4 affect both loops. + +## Current state + +### `src/Nodes/BodyNode.php` — the node-walking loops + +Exact code today, `src/Nodes/BodyNode.php:45-127`: + +```php + /** + * @throws LiquidException + */ + public function render(RenderContext $context): string + { + $context->resourceLimits->incrementRenderScore(count($this->children)); + + $output = ''; + + foreach ($this->children as $node) { + try { + if ($node instanceof Tag) { + $node->ensureTagIsEnabled($context); + } + + $output .= $this->renderChild($context, $node); + } catch (UndefinedVariableException|UndefinedDropMethodException|UndefinedFilterException $exception) { + $context->handleError($exception, $node->lineNumber); + } catch (\Throwable $exception) { + $output .= $context->handleError($exception, $node->lineNumber); + } + + if ($context->hasInterrupt()) { + break; + } + } + + $context->resourceLimits->incrementWriteScore($output); + + return $output; + } + + /** + * @return \Generator + * + * @throws LiquidException + */ + public function stream(RenderContext $context): \Generator + { + $context->resourceLimits->incrementRenderScore(count($this->children)); + + foreach ($this->children as $node) { + try { + if ($node instanceof Tag) { + $node->ensureTagIsEnabled($context); + } + + foreach ($this->streamChild($context, $node) as $output) { + $context->resourceLimits->incrementWriteScore($output); + yield $output; + } + } catch (UndefinedVariableException|UndefinedDropMethodException|UndefinedFilterException $exception) { + $context->handleError($exception, $node->lineNumber); + } catch (\Throwable $exception) { + $output = $context->handleError($exception, $node->lineNumber); + $context->resourceLimits->incrementWriteScore($output); + yield $output; + } + + if ($context->hasInterrupt()) { + break; + } + } + } + + protected function renderChild(RenderContext $context, Node $node): string + { + return $node->render($context); + } + + /** + * @return \Generator + */ + public function streamChild(RenderContext $context, Node $node): \Generator + { + if ($node instanceof CanBeStreamed) { + yield from $node->stream($context); + + return; + } + + yield $node->render($context); + } +``` + +The file's current imports (`src/Nodes/BodyNode.php:5-11`): + +```php +use Keepsuit\Liquid\Contracts\CanBeStreamed; +use Keepsuit\Liquid\Exceptions\LiquidException; +use Keepsuit\Liquid\Exceptions\UndefinedDropMethodException; +use Keepsuit\Liquid\Exceptions\UndefinedFilterException; +use Keepsuit\Liquid\Exceptions\UndefinedVariableException; +use Keepsuit\Liquid\Render\RenderContext; +use Keepsuit\Liquid\Tag; +``` + +### `src/Tag.php` — why the `Disableable` check is equivalent + +```php + public function ensureTagIsEnabled(RenderContext $context): void + { + if (! $this instanceof Disableable) { + return; + } + + if (! $context->tagDisabled(static::tagName())) { + return; + } + + throw new TagDisabledException(static::tagName()); + } +``` + +The method is a no-op unless `$this instanceof Disableable`. `Disableable` lives +at `Keepsuit\Liquid\Contracts\Disableable`. + +### `src/Render/RenderContext.php` — the interrupt check + +```php + public function hasInterrupt(): bool + { + return count($this->interrupts) > 0; + } +``` + +`$this->interrupts` is declared as `protected array $interrupts = [];` in the +same file, so `!== []` is exactly equivalent to `count(...) > 0`. + +### Conventions + +PHP 8.2+, PHPStan level 9 over `src/`, Laravel Pint. Note the level-9 +constraint on step 2: `ensureTagIsEnabled()` is declared on `Tag`, not on +`Disableable`, so testing `Disableable` alone makes PHPStan reject the call. +The plan's code keeps both checks — `Disableable` first (it filters out almost +everything at zero cost), `Tag` second (it satisfies the analyser). + +## Commands you will need + +| Purpose | Command | Expected on success | +|---------|---------|---------------------| +| Tests | `vendor/bin/pest` | `Tests: 800 passed` (or more), 0 failures | +| Static analysis | `vendor/bin/phpstan analyse --no-progress` | `[OK] No errors` | +| Formatting | `vendor/bin/pint --test` | exit 0 | +| Benchmark | see "Benchmark procedure" | render/stream both improve | + +## Benchmark procedure + +```bash +vendor/bin/phpbench run \ + --filter='benchRender|benchStream' \ + --progress=none \ + --warmup=1 \ + --retry-threshold=5 \ + --report=aggregate \ + --output=json > build/plan-003.json + +php tools/phpbench-compare.php build/base.json build/plan-003.json +``` + +Versus `build/base.json` this is the **cumulative** result of all three plans: +expected around **+19% on `benchRender` and +26% on `benchStream`**. + +To isolate this plan: `php tools/phpbench-compare.php build/plan-002.json build/plan-003.json` +→ expected roughly **+5% render, +9% stream**. Skip the isolated comparison if +`build/plan-002.json` does not exist. + +Do not commit anything under `build/` other than the already-committed +`base.json`. + +## Scope + +**In scope** (the only files you should modify): +- `src/Nodes/BodyNode.php` +- `src/Render/RenderContext.php` (the `hasInterrupt()` method only) +- `plans/README.md` (status row only) + +**Out of scope** (do NOT touch, even though they look related): +- `src/Tag.php` — `ensureTagIsEnabled()` keeps its internal `Disableable` + guard. It is public API and is called from elsewhere; the call-site check is + an addition, not a replacement. +- **Deleting `renderChild()` or `streamChild()`** — they become unused, but + `streamChild()` is `public` and `renderChild()` is `protected`, so removing + either is a breaking change for any downstream subclass. Leave both in place; + see "Maintenance notes". +- The `try`/`catch` structure and the error-handling branches in both loops — + do not restructure them. +- `incrementRenderScore()` / `incrementWriteScore()` call sites and their + ordering — the stream loop must keep scoring each chunk before yielding it. +- `src/Render/RenderContext.php` beyond `hasInterrupt()`. + +## Git workflow + +- Work on branch `perf/render-stream-optimizations` (created by plan 001; create + it from `main` with `git switch -c perf/render-stream-optimizations` if it + does not exist). +- One commit for this plan. Repo message style is plain sentence case, no + conventional-commit prefixes. Use: `Trim per-child overhead in body rendering`. +- Do NOT push and do NOT open a PR. + +## Steps + +### Step 1: Inline the non-streamable child case in `stream()` + +In `src/Nodes/BodyNode.php::stream()`, replace: + +```php + foreach ($this->streamChild($context, $node) as $output) { + $context->resourceLimits->incrementWriteScore($output); + yield $output; + } +``` + +with: + +```php + if ($node instanceof CanBeStreamed) { + foreach ($node->stream($context) as $output) { + $context->resourceLimits->incrementWriteScore($output); + yield $output; + } + } else { + $output = $node->render($context); + $context->resourceLimits->incrementWriteScore($output); + yield $output; + } +``` + +This is the same dispatch `streamChild()` performed, minus the intermediate +generator. Both branches stay inside the existing `try` block, so error handling +is unchanged. + +**Verify**: `vendor/bin/pest --filter=Stream` → all stream tests pass +(`tests/Integration/StreamTest.php` covers chunk boundaries, which is exactly +what this step could break). + +### Step 2: Check `Disableable` at the call site in both loops + +In `src/Nodes/BodyNode.php`, in **both** `render()` and `stream()`, replace: + +```php + if ($node instanceof Tag) { + $node->ensureTagIsEnabled($context); + } +``` + +with: + +```php + if ($node instanceof Disableable && $node instanceof Tag) { + $node->ensureTagIsEnabled($context); + } +``` + +Keep that operand order: `Disableable` first is the point of the change, `Tag` +second is what keeps PHPStan level 9 happy about the method call. + +Add the import alongside the existing ones at the top of the file: + +```php +use Keepsuit\Liquid\Contracts\Disableable; +``` + +Keep `use Keepsuit\Liquid\Tag;` — it is still referenced by the second operand. + +**Verify**: +- `vendor/bin/phpstan analyse --no-progress` → `[OK] No errors` +- `vendor/bin/pest` → all pass. Disabled-tag behaviour is covered by the + existing suite; if a test about disabled tags fails here, the operand order or + the `Disableable` import is wrong. + +### Step 3: Drop the `renderChild()` indirection at the call site + +In `src/Nodes/BodyNode.php::render()`, replace: + +```php + $output .= $this->renderChild($context, $node); +``` + +with: + +```php + $output .= $node->render($context); +``` + +Leave the `renderChild()` method definition in the file — see "Out of scope". + +**Verify**: `vendor/bin/pest` → all pass. + +### Step 4: Simplify `hasInterrupt()` + +In `src/Render/RenderContext.php`: + +```php + public function hasInterrupt(): bool + { + return $this->interrupts !== []; + } +``` + +**Verify**: `vendor/bin/pest --filter='break|continue'` → interrupt-related +tests pass. Then `vendor/bin/pest` → full suite green. + +### Step 5: Formatting, benchmark, commit + +```bash +vendor/bin/pint --test +``` + +Then run the "Benchmark procedure" above and record both delta percentages. + +```bash +git status +git add src/Nodes/BodyNode.php src/Render/RenderContext.php plans/README.md +git commit -m "Trim per-child overhead in body rendering" +``` + +**Verify**: `git show --stat HEAD` → exactly three files. + +## Test plan + +No new tests are required — every step is behaviour-preserving and the existing +suite already covers each affected path: + +- `tests/Integration/StreamTest.php` — streaming chunk boundaries. Step 1 + changes how chunks are produced, so this is the file that would catch a + mistake there. Note that streaming semantics include *how many* chunks a + template yields (the tests assert `toHaveCount(2)` and per-index values), so + a merged or split chunk fails loudly. That is intended: step 1 must not change + chunk boundaries. +- `tests/Unit/Tags/` and `tests/Integration/Tags/` — disabled-tag behaviour for + step 2. +- `tests/Unit/ResourceLimitsTest.php` — render/write scoring, which step 1 + moves the call sites of. +- Break/continue interrupt tests for step 4. + +If you would like one extra guard, add a test to `tests/Integration/StreamTest.php` +asserting that a template mixing text, an `{% if %}` block, and an output tag +streams the same chunk sequence before and after — but only if you can write it +without changing any existing test. + +**Verification**: `vendor/bin/pest` → all pass, count unchanged (or +1). + +## Done criteria + +ALL must hold: + +- [ ] `vendor/bin/pest` exits 0 with the same test count as before (or +1) +- [ ] `vendor/bin/phpstan analyse --no-progress` reports `[OK] No errors` +- [ ] `vendor/bin/pint --test` exits 0 +- [ ] `grep -n 'streamChild($context' src/Nodes/BodyNode.php` returns no matches (the call is gone; the method definition remains) +- [ ] `grep -n 'renderChild($context' src/Nodes/BodyNode.php` returns no matches +- [ ] `grep -n 'count($this->interrupts)' src/Render/RenderContext.php` returns no matches +- [ ] `grep -n 'protected function renderChild' src/Nodes/BodyNode.php` still returns a match (kept for BC) +- [ ] `php tools/phpbench-compare.php build/base.json build/plan-003.json` shows a positive delta on both subjects +- [ ] `git show --stat HEAD` lists only the three in-scope files +- [ ] `plans/README.md` status row for 003 updated to DONE + +## STOP conditions + +Stop and report back (do not improvise) if: + +- `src/Nodes/BodyNode.php` differs from the "Current state" excerpt. +- Any test in `tests/Integration/StreamTest.php` fails — that means step 1 + changed chunk boundaries, which it must not. +- PHPStan reports an error about `ensureTagIsEnabled()` not existing. The fix is + the operand order given in step 2; if that does not resolve it, stop rather + than adding a baseline entry or a `@phpstan-ignore` comment. +- Any test fails and the only way you can see to make it pass is editing the + test. +- The benchmark shows a negative delta on either subject across two runs. + +## Maintenance notes + +- `renderChild()` and `streamChild()` are now dead code kept only for backward + compatibility with downstream subclasses. Neither has an override anywhere in + this repo. They are the right thing to delete in the next major release — flag + that to the maintainer rather than doing it here. +- Because `streamChild()` is no longer called, a subclass that overrode it to + customise streaming would silently stop taking effect. That is the one real + behavioural risk in this plan, and it is why deleting the method now (which + would fail loudly instead) is a defensible alternative the maintainer may + prefer. Raise it; do not decide it yourself. +- The `Disableable && Tag` check duplicates the guard inside + `Tag::ensureTagIsEnabled()`. If that method ever grows behaviour that must run + for non-`Disableable` tags, the call-site check has to come out again. +- Deferred: coalescing runs of adjacent `Text` nodes at parse time would cut the + per-child cost further for both loops, but it is a parser-phase change and was + out of scope for this render-phase audit. diff --git a/plans/README.md b/plans/README.md new file mode 100644 index 0000000..48f6d86 --- /dev/null +++ b/plans/README.md @@ -0,0 +1,92 @@ +# Implementation Plans + +Generated by the improve skill on 2026-07-29 against commit `4a1bdd6`. Focus: +performance of the **render / streaming** phase only. Execute in the order +below — the measurements were taken by stacking them in this order, and each +plan's expected gain assumes the previous ones have landed. + +Each executor: read the plan fully before starting, honor its STOP conditions, +and update your row when done. + +## Execution order & status + +| Plan | Title | Priority | Effort | Depends on | Status | +|------|-------|----------|--------|------------|--------| +| 001 | Fast-path array scopes in `RenderContext::internalContextLookup()` | P1 | S | — | TODO | +| 002 | Stop scanning every scope: lazy `RenderContext::iterateVariables()` | P1 | S | 001 (order only) | TODO | +| 003 | Remove per-child overhead in `BodyNode` render/stream loops | P1 | S | 001, 002 (order only) | TODO | + +Status values: TODO | IN PROGRESS | DONE | BLOCKED (with one-line reason) | REJECTED (with one-line rationale) + +## Dependency notes + +- No plan is technically blocked by another — they touch overlapping but + non-conflicting hunks. The ordering matters only because the measured deltas + below were taken cumulatively in this order, and because 002 and 003 both + touch `src/Nodes/BodyNode.php` / `src/Render/RenderContext.php`; running them + out of order means resolving trivial context drift by hand. + +## Measured baseline and expected outcome + +All numbers below were measured on a scratch copy of this repo at `4a1bdd6`, +PHP 8.5.9, opcache on, JIT off, using an interleaved A/B harness (15 runs per +side per round, minimum-of-run reported, 3+ rounds). "render"/"stream" are one +full pass of `ThemeRunner::render()` / `ThemeRunner::stream()` over the four +theme fixtures in `performance/tests/`. + +| Stage | render (µs, min) | stream (µs, min) | +|-------|-----------------:|-----------------:| +| baseline `4a1bdd6` | 4008 | 4312 | +| + plan 001 | 3506 (−12.5%) | 3823 (−11.3%) | +| + plan 002 | 3239 (−7.6%) | 3560 (−6.9%) | +| + plan 003 | 3047 (−5.9%) | 3226 (−9.4%) | + +Confirmed with the real benchmark harness (`vendor/bin/phpbench`, 10 revs × +10 its) against `build/base.json`: + +| Subject | Base | All three plans | Delta | +|---------|-----:|----------------:|------:| +| `LiquidBench::benchRender` | 208.4 ops/s | 249.5 ops/s | **+19.7%** | +| `LiquidBench::benchStream` | 190.3 ops/s | 240.8 ops/s | **+26.5%** | + +Peak memory is unchanged (6.13 MB both sides). The full test suite +(800 tests, 1990 assertions) and `phpstan analyse` at level 9 pass with all +three plans applied. + +## Call-count evidence + +Instrumented counters for one full `ThemeRunner::render()` pass at `4a1bdd6`: + +| Counter | Count | +|---------|------:| +| `BodyNode::render()` calls | 361 | +| child nodes rendered | 4384 | +| `RenderContext::findVariables()` calls | 1862 | +| scope probes performed by those calls | 7394 | +| `RenderContext::internalContextLookup()` calls | 8989 | +| `RenderContext::normalizeValue()` calls | 9046 | +| `RenderContext::set()` calls | 234 | +| `Drop::__get()` calls | 81 | + +## Findings considered and rejected + +- **Early-return for non-objects in `RenderContext::normalizeValue()`**: measured + as zero delta across three interleaved rounds. The existing + `is_object($value) && isset($this->sharedState->computedObjectsCache[$value])` + already short-circuits on the first check for scalars and arrays. +- **Per-class name-resolution cache in `Drop::__get()`** (memoize the + `Str::camel`/`Str::snake`/`array_unique`/`in_array` resolution per class): + correct in principle, but `Drop::__get()` is called only 81 times in the whole + theme benchmark, so it is unmeasurable here — the theme fixtures feed plain + arrays from `performance/Shopify/vision.database.yml`, not drops. Revisit only + after adding a drop-heavy benchmark fixture; without one there is no way to + verify the change helps. +- **`RenderContext::set()` / `Arr::set()` dotted-key parsing**: only 234 calls + per render pass. Not worth touching. +- **`ResourceLimits::incrementRenderScore()` per body**: 361 calls per pass. + Negligible. + +## Not audited + +Parse/tokenize phase, filter implementations, template caching, and the +`Profiler` — this run was scoped to render and streaming. From 804e3b9bf636c486a85d12c1d0e4fd72cf1c9a1d Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 11:38:48 +0200 Subject: [PATCH 02/18] Fast-path array scopes in context lookup --- plans/README.md | 2 +- src/Render/RenderContext.php | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/plans/README.md b/plans/README.md index 48f6d86..eab4ec9 100644 --- a/plans/README.md +++ b/plans/README.md @@ -12,7 +12,7 @@ and update your row when done. | Plan | Title | Priority | Effort | Depends on | Status | |------|-------|----------|--------|------------|--------| -| 001 | Fast-path array scopes in `RenderContext::internalContextLookup()` | P1 | S | — | TODO | +| 001 | Fast-path array scopes in `RenderContext::internalContextLookup()` | P1 | S | — | DONE | | 002 | Stop scanning every scope: lazy `RenderContext::iterateVariables()` | P1 | S | 001 (order only) | TODO | | 003 | Remove per-child overhead in `BodyNode` render/stream loops | P1 | S | 001, 002 (order only) | TODO | diff --git a/src/Render/RenderContext.php b/src/Render/RenderContext.php index 65cf862..89fc327 100644 --- a/src/Render/RenderContext.php +++ b/src/Render/RenderContext.php @@ -220,10 +220,17 @@ public function getSelfDrop(): SelfDrop public function internalContextLookup(mixed $scope, int|string $key): mixed { + if (is_array($scope)) { + if (! array_key_exists($key, $scope)) { + return $this->missingValue; + } + + return $this->normalizeValue($scope[$key]); + } + try { $value = match (true) { $scope instanceof Drop => $scope->{$key}, - is_array($scope) && array_key_exists($key, $scope) => $scope[$key], is_object($scope) && $this->objectHasProperty($scope, (string) $key) => $scope->{$key}, is_object($scope) && $this->objectHasStaticProperty($scope, (string) $key) => $scope::$$key, default => $this->missingValue, From a4d85b8568fe285fe6bd3a90214f8b869f365e6e Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 11:42:19 +0200 Subject: [PATCH 03/18] Resolve variables lazily instead of scanning every scope --- plans/README.md | 2 +- src/Nodes/VariableLookup.php | 8 +++--- src/Render/RenderContext.php | 34 +++++++++++++--------- tests/Integration/ContextTest.php | 47 +++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 19 deletions(-) diff --git a/plans/README.md b/plans/README.md index eab4ec9..ae21c48 100644 --- a/plans/README.md +++ b/plans/README.md @@ -13,7 +13,7 @@ and update your row when done. | Plan | Title | Priority | Effort | Depends on | Status | |------|-------|----------|--------|------------|--------| | 001 | Fast-path array scopes in `RenderContext::internalContextLookup()` | P1 | S | — | DONE | -| 002 | Stop scanning every scope: lazy `RenderContext::iterateVariables()` | P1 | S | 001 (order only) | TODO | +| 002 | Stop scanning every scope: lazy `RenderContext::iterateVariables()` | P1 | S | 001 (order only) | DONE | | 003 | Remove per-child overhead in `BodyNode` render/stream loops | P1 | S | 001, 002 (order only) | TODO | Status values: TODO | IN PROGRESS | DONE | BLOCKED (with one-line reason) | REJECTED (with one-line rationale) diff --git a/src/Nodes/VariableLookup.php b/src/Nodes/VariableLookup.php index 4e7d2b8..f5c2e98 100644 --- a/src/Nodes/VariableLookup.php +++ b/src/Nodes/VariableLookup.php @@ -90,14 +90,14 @@ public function evaluate(RenderContext $context): mixed { $name = $context->evaluate($this->name); assert(is_string($name)); - $variables = $context->findVariables($name); + $variables = $context->iterateVariables($name); if ($this->lookups === []) { - if ($context->options->strictVariables && $variables === []) { - return new UndefinedVariable($this->toString()); + foreach ($variables as $variable) { + return $variable; } - return $variables[0] ?? null; + return $context->options->strictVariables ? new UndefinedVariable($this->toString()) : null; } foreach ($variables as $object) { diff --git a/src/Render/RenderContext.php b/src/Render/RenderContext.php index 89fc327..cb88ca5 100644 --- a/src/Render/RenderContext.php +++ b/src/Render/RenderContext.php @@ -179,7 +179,15 @@ public function has(string $key): bool public function findVariables(string $key): array { - $variables = []; + return iterator_to_array($this->iterateVariables($key), preserve_keys: false); + } + + /** + * @return \Generator + */ + public function iterateVariables(string $key): \Generator + { + $found = false; // Check the variable in all scopes + env data + static variables $scopeCount = count($this->scopes); @@ -193,24 +201,22 @@ public function findVariables(string $key): array $value = $this->internalContextLookup($scope, $key); if (! $value instanceof MissingValue) { - $variables[] = $value; - } - } + $found = true; - // Inject the implicit self drop only when no value (including explicit null) was found. - // An explicit `self = nil` leaves [null] in $variables, so the fallback is skipped, - // correctly distinguishing defined-null from undefined. - if ($variables === [] && $key === 'self') { - return [$this->getSelfDrop()]; - } + if ($value instanceof IsContextAware) { + $value->setContext($this); + } - foreach ($variables as $variable) { - if ($variable instanceof IsContextAware) { - $variable->setContext($this); + yield $value; } } - return $variables; + // Inject the implicit self drop only when no value (including explicit null) was found. + // An explicit `self = nil` yields null before this point, so $found is true and the + // fallback is skipped, correctly distinguishing defined-null from undefined. + if (! $found && $key === 'self') { + yield $this->getSelfDrop(); + } } public function getSelfDrop(): SelfDrop diff --git a/tests/Integration/ContextTest.php b/tests/Integration/ContextTest.php index 6cbb088..35e4911 100644 --- a/tests/Integration/ContextTest.php +++ b/tests/Integration/ContextTest.php @@ -735,3 +735,50 @@ function () use (&$global) { 'default' => false, 'strict' => true, ]); + +test('lazy variable resolution returns the innermost scope', function (bool $strict) { + $context = new RenderContext(options: new RenderContextOptions(strictVariables: $strict)); + $context->set('test', 'outer'); + + $context->stack(function () use ($context) { + $context->set('test', 'inner'); + + expect($context->get('test'))->toBe('inner'); + }); + + expect($context->get('test'))->toBe('outer'); +})->with([ + 'default' => false, + 'strict' => true, +]); + +test('lazy variable resolution distinguishes defined-null from undefined', function (bool $strict) { + $context = new RenderContext(options: new RenderContextOptions(strictVariables: $strict)); + $context->set('definedNull', null); + + expect($context->get('definedNull'))->toBe(null); + + if ($strict) { + expect($context->get('neverSet'))->toBeInstanceOf(UndefinedVariable::class); + } else { + expect($context->get('neverSet'))->toBe(null); + } +})->with([ + 'default' => false, + 'strict' => true, +]); + +test('lazy variable resolution falls back when a lookup path misses', function (bool $strict) { + $context = new RenderContext(options: new RenderContextOptions(strictVariables: $strict)); + $context->set('test', ['b' => 'outer']); + + $context->stack(function () use ($context) { + $context->set('test', ['c' => 'inner']); + + expect($context->get('test.c'))->toBe('inner'); + expect($context->get('test.b'))->toBe('outer'); + }); +})->with([ + 'default' => false, + 'strict' => true, +]); From 24a34bb1a13d496558f688f94b568bd2a608811d Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 11:44:31 +0200 Subject: [PATCH 04/18] Trim per-child overhead in body rendering Co-Authored-By: Claude Opus 5 --- plans/README.md | 2 +- src/Nodes/BodyNode.php | 15 +++++++++++---- src/Render/RenderContext.php | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/plans/README.md b/plans/README.md index ae21c48..56dc80b 100644 --- a/plans/README.md +++ b/plans/README.md @@ -14,7 +14,7 @@ and update your row when done. |------|-------|----------|--------|------------|--------| | 001 | Fast-path array scopes in `RenderContext::internalContextLookup()` | P1 | S | — | DONE | | 002 | Stop scanning every scope: lazy `RenderContext::iterateVariables()` | P1 | S | 001 (order only) | DONE | -| 003 | Remove per-child overhead in `BodyNode` render/stream loops | P1 | S | 001, 002 (order only) | TODO | +| 003 | Remove per-child overhead in `BodyNode` render/stream loops | P1 | S | 001, 002 (order only) | DONE | Status values: TODO | IN PROGRESS | DONE | BLOCKED (with one-line reason) | REJECTED (with one-line rationale) diff --git a/src/Nodes/BodyNode.php b/src/Nodes/BodyNode.php index 60aa104..4a0f89d 100644 --- a/src/Nodes/BodyNode.php +++ b/src/Nodes/BodyNode.php @@ -3,6 +3,7 @@ namespace Keepsuit\Liquid\Nodes; use Keepsuit\Liquid\Contracts\CanBeStreamed; +use Keepsuit\Liquid\Contracts\Disableable; use Keepsuit\Liquid\Exceptions\LiquidException; use Keepsuit\Liquid\Exceptions\UndefinedDropMethodException; use Keepsuit\Liquid\Exceptions\UndefinedFilterException; @@ -53,11 +54,11 @@ public function render(RenderContext $context): string foreach ($this->children as $node) { try { - if ($node instanceof Tag) { + if ($node instanceof Disableable && $node instanceof Tag) { $node->ensureTagIsEnabled($context); } - $output .= $this->renderChild($context, $node); + $output .= $node->render($context); } catch (UndefinedVariableException|UndefinedDropMethodException|UndefinedFilterException $exception) { $context->handleError($exception, $node->lineNumber); } catch (\Throwable $exception) { @@ -85,11 +86,17 @@ public function stream(RenderContext $context): \Generator foreach ($this->children as $node) { try { - if ($node instanceof Tag) { + if ($node instanceof Disableable && $node instanceof Tag) { $node->ensureTagIsEnabled($context); } - foreach ($this->streamChild($context, $node) as $output) { + if ($node instanceof CanBeStreamed) { + foreach ($node->stream($context) as $output) { + $context->resourceLimits->incrementWriteScore($output); + yield $output; + } + } else { + $output = $node->render($context); $context->resourceLimits->incrementWriteScore($output); yield $output; } diff --git a/src/Render/RenderContext.php b/src/Render/RenderContext.php index cb88ca5..947c157 100644 --- a/src/Render/RenderContext.php +++ b/src/Render/RenderContext.php @@ -345,7 +345,7 @@ public function popInterrupt(): ?Interrupt public function hasInterrupt(): bool { - return count($this->interrupts) > 0; + return $this->interrupts !== []; } /** From adbadfc19912420e14f9ab7910201b1169044e13 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 13:07:52 +0200 Subject: [PATCH 05/18] refactoring --- src/Nodes/BodyNode.php | 19 ------------------ src/Render/RenderContext.php | 37 ++++++++++++++++++++---------------- 2 files changed, 21 insertions(+), 35 deletions(-) diff --git a/src/Nodes/BodyNode.php b/src/Nodes/BodyNode.php index 4a0f89d..1a16e19 100644 --- a/src/Nodes/BodyNode.php +++ b/src/Nodes/BodyNode.php @@ -114,25 +114,6 @@ public function stream(RenderContext $context): \Generator } } - protected function renderChild(RenderContext $context, Node $node): string - { - return $node->render($context); - } - - /** - * @return \Generator - */ - public function streamChild(RenderContext $context, Node $node): \Generator - { - if ($node instanceof CanBeStreamed) { - yield from $node->stream($context); - - return; - } - - yield $node->render($context); - } - public function blank(): bool { foreach ($this->children as $node) { diff --git a/src/Render/RenderContext.php b/src/Render/RenderContext.php index 947c157..60b6a58 100644 --- a/src/Render/RenderContext.php +++ b/src/Render/RenderContext.php @@ -200,15 +200,17 @@ public function iterateVariables(string $key): \Generator $value = $this->internalContextLookup($scope, $key); - if (! $value instanceof MissingValue) { - $found = true; + if ($value instanceof MissingValue) { + continue; + } - if ($value instanceof IsContextAware) { - $value->setContext($this); - } + $found = true; - yield $value; + if ($value instanceof IsContextAware) { + $value->setContext($this); } + + yield $value; } // Inject the implicit self drop only when no value (including explicit null) was found. @@ -226,19 +228,18 @@ public function getSelfDrop(): SelfDrop public function internalContextLookup(mixed $scope, int|string $key): mixed { - if (is_array($scope)) { - if (! array_key_exists($key, $scope)) { - return $this->missingValue; - } - - return $this->normalizeValue($scope[$key]); - } - try { $value = match (true) { + is_array($scope) => match (true) { + array_key_exists($key, $scope) => $scope[$key], + default => $this->missingValue, + }, $scope instanceof Drop => $scope->{$key}, - is_object($scope) && $this->objectHasProperty($scope, (string) $key) => $scope->{$key}, - is_object($scope) && $this->objectHasStaticProperty($scope, (string) $key) => $scope::$$key, + is_object($scope) => match (true) { + $this->objectHasProperty($scope, (string) $key) => $scope->{$key}, + $this->objectHasStaticProperty($scope, (string) $key) => $scope::$$key, + default => $this->missingValue, + }, default => $this->missingValue, }; } catch (UndefinedDropMethodException) { @@ -277,6 +278,10 @@ protected function objectHasStaticProperty(object $object, string $property): bo public function normalizeValue(mixed $value): mixed { + if ($value instanceof MissingValue) { + return $value; + } + if (is_object($value) && isset($this->sharedState->computedObjectsCache[$value])) { return $this->sharedState->computedObjectsCache[$value]; } From d7f82018923f32cf8828d1d09c55b9a4f05acfa8 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 13:16:28 +0200 Subject: [PATCH 06/18] Add plan for variable lookup type fix Co-Authored-By: Claude Opus 5 --- ...lelookup-dead-evaluate-and-lookup-types.md | 470 ++++++++++++++++++ plans/README.md | 1 + 2 files changed, 471 insertions(+) create mode 100644 plans/004-variablelookup-dead-evaluate-and-lookup-types.md diff --git a/plans/004-variablelookup-dead-evaluate-and-lookup-types.md b/plans/004-variablelookup-dead-evaluate-and-lookup-types.md new file mode 100644 index 0000000..04588ed --- /dev/null +++ b/plans/004-variablelookup-dead-evaluate-and-lookup-types.md @@ -0,0 +1,470 @@ +# Plan 004: Drop the dead `evaluate()` call in `VariableLookup` and correct the `$lookups` type + +> **Executor instructions**: Follow this plan step by step. Run every +> verification command and confirm the expected result before moving to the +> next step. If anything in the "STOP conditions" section occurs, stop and +> report — do not improvise. When done, update the status row for this plan +> in `plans/README.md`. +> +> **Drift check (run first)**: `git diff --stat 24a34bb..HEAD -- src/Nodes/VariableLookup.php` +> If that file changed since this plan was written, compare the "Current state" +> excerpts against the live code before proceeding; on a mismatch, treat it as a +> STOP condition. + +## Status + +- **Priority**: P2 +- **Effort**: S +- **Risk**: LOW +- **Depends on**: plans/002-lazy-variable-lookup.md (already DONE — this plan edits code that plan introduced) +- **Category**: perf + bug +- **Planned at**: commit `24a34bb`, 2026-07-29 + +## Why this matters + +Two things, both in `src/Nodes/VariableLookup.php`, discovered together because +one hid the other. + +**The performance half.** `RenderContext::evaluate()` is the highest-volume +function in the render path — 11145 calls per pass of the theme benchmark, of +which 9283 (83%) do nothing but one `instanceof` check and a return. +`evaluate($this->name)` accounts for 1862 of those, and it can *never* do +anything: `VariableLookup::$name` is declared `public readonly string`, a real +PHP type declaration the engine enforces, and a string is never +`CanBeEvaluated`. There is even an `assert(is_string($name))` on the next line +confirming the intent. Deleting the call measured **−3.2% render, −2.6% stream** +over three interleaved A/B rounds. + +**The correctness half.** `$lookups` is annotated `/** @var string[] */`, and +that is **wrong**. `ExpressionParser::parseVariableLookups()` +(`src/Parse/ExpressionParser.php:64-87`) pushes a plain string for a dot lookup +(`a.b`) but pushes `$this->tokenStream->expression()` for a bracket lookup +(`a[b]`) — which can be a `VariableLookup`, `RangeLookup`, `Literal`, int, +float, bool, or null. PHPStan has been reasoning from a false premise about this +property, and it hides a reachable crash: + +``` +{{ a[empty] }} with strictVariables: true + → InternalException (wrapping "Object of class Literal could not be converted to string") +``` + +instead of the `UndefinedVariableException: Variable 'a.empty' not found` that +`{{ a[b] }}` and `{{ a[(1..2)] }}` correctly produce. The cause is +`toString()` calling `implode()` over `$this->lookups`; `Literal` is a backed +enum with no `__toString()`, so the implode throws. `VariableLookup` and +`RangeLookup` happen to survive only because they define `__toString()`. + +Fixing the annotation makes PHPStan surface exactly two real problems, both of +which this plan fixes properly. + +## Current state + +Everything changes in one file: `src/Nodes/VariableLookup.php`. + +The class header and imports today (`src/Nodes/VariableLookup.php:1-16`): + +```php +lookups === []) { + return $this->name; + } + + return implode('.', [$this->name, ...$this->lookups]); + } +``` + +The head of `evaluate()` (`src/Nodes/VariableLookup.php:89-93`): + +```php + public function evaluate(RenderContext $context): mixed + { + $name = $context->evaluate($this->name); + assert(is_string($name)); + $variables = $context->iterateVariables($name); +``` + +The filter-fallback branch inside the lookup loop +(`src/Nodes/VariableLookup.php:110-119`): + +```php + foreach ($this->lookups as $i => $lookup) { + $key = $context->evaluate($lookup) ?? ''; + + assert(is_string($key) || is_int($key)); + + $nextObject = $context->evaluate($context->internalContextLookup($object, $key)); + + if ($nextObject instanceof MissingValue && is_iterable($object) && in_array($i, $this->lookupFilters, true)) { + $nextObject = $context->applyFilter($lookup, $object); + } +``` + +### Facts you must not get wrong + +- **`$context->evaluate($lookup)` on line 111 is NOT dead and must stay.** + It is what resolves the variable inside a bracket lookup like `{{ a[b] }}`. + Removing it breaks 7 tests. Only the `evaluate($this->name)` call on line 91 + is dead. +- `$lookupFilters` (built in the constructor) holds only the indices `$i` where + the lookup is one of the strings in `FILTER_METHODS`. So inside the + `in_array($i, $this->lookupFilters, true)` branch, `$lookup` is always a + string at runtime — PHPStan just cannot see it. Narrow it with an explicit + `is_string($lookup)` in the condition, which is honest rather than a + suppression. +- The `Expression` type alias is declared on `ExpressionParser` + (`src/Parse/ExpressionParser.php:11`) as: + `string|int|float|bool|Literal|VariableLookup|RangeLookup|null`. + Other files in this repo import it with + `@phpstan-import-type Expression from ExpressionParser` — see the class + docblock of `src/Nodes/Variable.php` and `src/Tags/ForTag.php` for the exact + convention to copy. +- `Literal` (`src/Nodes/Literal.php`) is a **backed enum** (`case Empty = 'empty';` + `case Blank = 'blank';`), so its string form is `$literal->value`. +- `toString()` is on the cold path only — it is called from the two + `strictVariables` error branches and from `__toString()`. Readability beats + micro-optimization there. + +### Conventions + +PHP 8.2+, PHPStan level 9 over `src/`, Laravel Pint. This repo's CI forbids +silencing analysis errors: **do not add `@phpstan-ignore` comments, baseline +entries, inline `@var` overrides, or type casts to make an error go away.** Fix +the underlying cause. `match (true)` chains are the idiom used across this +codebase for this kind of dispatch — see `Variable::debugLabel()` in +`src/Nodes/Variable.php:136-146` for a near-identical shape you should mirror. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---------|---------|---------------------| +| Tests | `vendor/bin/pest` | `Tests: 806 passed` (or more), 0 failures | +| Static analysis | `vendor/bin/phpstan analyse --no-progress` | `[OK] No errors` | +| Formatting | `vendor/bin/pint --test` | exit 0 | +| Benchmark | see "Benchmark procedure" | render/stream both improve | + +## Benchmark procedure + +```bash +vendor/bin/phpbench run \ + --filter='benchRender|benchStream' \ + --progress=none \ + --warmup=1 \ + --retry-threshold=5 \ + --report=aggregate \ + --output=json > build/plan-004.json + +php tools/phpbench-compare.php build/base.json build/plan-004.json +``` + +**Important expectation-setting**: `build/base.json` was captured on CI +hardware, so the absolute percentages it produces locally are inflated and not +meaningful. What matters here is only that neither subject *regresses*. This +plan's own contribution measured **−3.2% render / −2.6% stream** in a controlled +same-machine A/B, which is close to the ±2% band the compare tool treats as +noise — so a local single-run comparison may well show this plan as neutral. +**That is an acceptable outcome and is NOT a STOP condition.** Only a clear +regression (worse than −5% on either subject, reproduced across two runs) is. + +Do not commit anything under `build/` other than the already-committed +`base.json`. + +## Scope + +**In scope** (the only files you should modify): +- `src/Nodes/VariableLookup.php` +- `tests/Integration/VariableTest.php` (add the regression test — see "Test plan") +- `plans/README.md` (status row only) + +**Out of scope** (do NOT touch, even though they look related): +- `$context->evaluate($lookup)` on line 111 — see "Facts you must not get + wrong". It is load-bearing. +- `$context->evaluate($object)` at the top of the `foreach ($variables ...)` + loop — scope values genuinely can be `CanBeEvaluated`. +- `src/Parse/ExpressionParser.php` — the parser is correct; it was the + annotation that was wrong. +- `src/Nodes/Variable.php`, `src/Nodes/RangeLookup.php`, `src/Nodes/Literal.php`. +- `src/Render/RenderContext.php::evaluate()` — the other ~5800 no-op calls come + from call sites whose types do not prove them dead. Not this plan. +- `phpstan-baseline.neon` — must not gain entries. +- `build/base.json`. + +## Git workflow + +- Work on the existing branch `perf/render-stream-optimizations`. Do NOT create + a branch and do NOT switch branches. +- One commit for this plan. Repo message style is plain sentence case, no + conventional-commit prefixes (see `git log --oneline`). Use: + `Fix variable lookup types and drop a dead evaluate call` +- Do NOT push and do NOT open a PR. A PR (#69) already exists for this branch; + pushing is the maintainer's call. + +## Steps + +### Step 1: Reproduce the bug first + +Confirm the defect exists before fixing it. Create a scratch file +`/tmp/repro-004.php` (outside the repo — do NOT add it to the repo): + +```php +build(); +$tpl = $env->parseString('{{ a[empty] }}'); +$ctx = new RenderContext( + options: new RenderContextOptions(strictVariables: true, rethrowErrors: true), + environment: $env, +); + +try { + $tpl->render($ctx); + echo "NO EXCEPTION\n"; +} catch (\Throwable $e) { + echo get_class($e).': '.$e->getMessage()."\n"; +} +``` + +**Verify**: `php /tmp/repro-004.php` → prints +`Keepsuit\Liquid\Exceptions\InternalException: Internal exception`. + +If it instead prints an `UndefinedVariableException`, the bug is already fixed — +STOP and report. + +### Step 2: Delete the dead `evaluate()` call + +In `evaluate()`, replace: + +```php + $name = $context->evaluate($this->name); + assert(is_string($name)); + $variables = $context->iterateVariables($name); +``` + +with: + +```php + $variables = $context->iterateVariables($this->name); +``` + +`$name` has no other use in the method — confirm that with +`grep -n '\$name' src/Nodes/VariableLookup.php` before deleting (the only other +hits should be `$this->name`). + +**Verify**: `vendor/bin/pest` → `Tests: 806 passed`, 0 failures. + +### Step 3: Correct the `$lookups` annotation + +Add the type import to the class docblock, immediately above +`class VariableLookup`: + +```php +/** + * @phpstan-import-type Expression from ExpressionParser + */ +class VariableLookup implements CanBeEvaluated, HasParseTreeVisitorChildren +``` + +and add the matching import alongside the existing `use` statements: + +```php +use Keepsuit\Liquid\Parse\ExpressionParser; +``` + +Then change the constructor annotation: + +```php + public function __construct( + public readonly string $name, + /** @var array */ + public readonly array $lookups = [], + ) { +``` + +**Verify**: `vendor/bin/phpstan analyse --no-progress` → **exactly 2 errors**, +both in `src/Nodes/VariableLookup.php`: +- one on the `implode` in `toString()` (`argument.type`) +- one on the `applyFilter` call in the lookup loop (`argument.type`) + +Those two are the real problems the wrong annotation was hiding; steps 4 and 5 +fix them. If you see a different number of errors, or errors in other files, +STOP and report. + +### Step 4: Make `toString()` handle every lookup type + +Replace `toString()` with: + +```php + public function toString(): string + { + if ($this->lookups === []) { + return $this->name; + } + + $lookups = array_map( + fn (mixed $lookup): string => match (true) { + is_string($lookup) => $lookup, + $lookup instanceof Literal => $lookup->value, + $lookup instanceof VariableLookup, $lookup instanceof RangeLookup => $lookup->toString(), + is_bool($lookup) => $lookup ? 'true' : 'false', + $lookup === null => '', + default => (string) $lookup, + }, + $this->lookups, + ); + + return implode('.', [$this->name, ...$lookups]); + } +``` + +`default` covers the remaining `int|float`. This mirrors the existing +`Variable::debugLabel()` idiom. + +**Verify**: `php /tmp/repro-004.php` → now prints +`Keepsuit\Liquid\Exceptions\UndefinedVariableException: Variable 'a.empty' not found` +(exact quoting of the variable name may differ — what matters is that it is an +`UndefinedVariableException` naming `a.empty`, not an `InternalException`). + +### Step 5: Narrow `$lookup` at the filter-fallback call + +In the lookup loop, change the condition: + +```php + if ($nextObject instanceof MissingValue && is_string($lookup) && is_iterable($object) && in_array($i, $this->lookupFilters, true)) { + $nextObject = $context->applyFilter($lookup, $object); + } +``` + +Keep `$nextObject instanceof MissingValue` as the first operand — it is the +cheapest and rarest test, and this is on the hot path. + +**Verify**: +- `vendor/bin/phpstan analyse --no-progress` → `[OK] No errors` +- `vendor/bin/pint --test` → exit 0 +- `vendor/bin/pest` → `Tests: 806 passed`, 0 failures + +### Step 6: Add the regression test + +See "Test plan". + +**Verify**: `vendor/bin/pest` → all pass, one more test than before. + +### Step 7: Benchmark and commit + +Run the "Benchmark procedure". Record both deltas, and remember that neutral is +an acceptable result here. + +```bash +git status +git add src/Nodes/VariableLookup.php tests/Integration/VariableTest.php plans/README.md +git commit -m "Fix variable lookup types and drop a dead evaluate call" +``` + +**Verify**: `git show --stat HEAD` → exactly three files. + +## Test plan + +Add one regression test to `tests/Integration/VariableTest.php`, pinning the bug +from step 1. Read two neighbouring tests in that file first and match their +structure and helper usage (the file uses Pest `test('...', function () { ... })` +with this repo's template helpers). + +The case: **a bracket lookup whose key is a `Literal`, under `strictVariables`, +reports an undefined variable rather than an internal error.** Concretely, +rendering `{{ a[empty] }}` with `strictVariables: true` must raise +`UndefinedVariableException`, not `InternalException`. + +If `tests/Integration/VariableTest.php` turns out not to be the natural home +(for example if bracket-lookup tests live in `tests/Integration/OutputTest.php` +or `tests/Integration/ContextTest.php`), put it wherever the existing +bracket-lookup tests are and say so in your report — matching the file's +neighbours matters more than the exact file named here. + +Do not add a test for the performance change; it is behaviour-preserving and +already covered. + +**Verification**: `vendor/bin/pest` → 807 passed (or more), 0 failures. + +## Done criteria + +ALL must hold: + +- [ ] `vendor/bin/pest` exits 0 with one more test than before (807+) +- [ ] `vendor/bin/phpstan analyse --no-progress` reports `[OK] No errors` +- [ ] `vendor/bin/pint --test` exits 0 +- [ ] `git diff HEAD~1 -- phpstan-baseline.neon` is empty (no new baseline entries) +- [ ] `grep -n 'assert(is_string($name))' src/Nodes/VariableLookup.php` returns no matches +- [ ] `grep -n '@var string\[\]' src/Nodes/VariableLookup.php` returns no matches +- [ ] `grep -n 'evaluate($lookup)' src/Nodes/VariableLookup.php` still returns a match (the load-bearing call was kept) +- [ ] `php /tmp/repro-004.php` prints an `UndefinedVariableException`, not an `InternalException` +- [ ] `php tools/phpbench-compare.php build/base.json build/plan-004.json` shows no clear regression on either subject +- [ ] `git show --stat HEAD` lists only the three in-scope files +- [ ] `plans/README.md` status row for 004 updated to DONE + +## STOP conditions + +Stop and report back (do not improvise) if: + +- The repro in step 1 does not reproduce. +- After step 3, PHPStan reports anything other than exactly the two expected + `argument.type` errors in `src/Nodes/VariableLookup.php`. +- You are tempted to add a `@phpstan-ignore` comment, a baseline entry, an + inline `@var`, or a cast to clear an analysis error. The repo forbids it — + stop instead. +- Removing the `evaluate($this->name)` call causes any test to fail. It should + not; if it does, the assumption that `$name` is always a `string` is false and + that changes the whole plan. +- You conclude that `$context->evaluate($lookup)` on line 111 should also be + removed. It must not be — that path is what 7 tests cover. +- The benchmark shows worse than −5% on either subject across two runs. + +## Maintenance notes + +- The `@var array` annotation is now the accurate contract. If + anyone "simplifies" it back to `string[]`, both bugs return silently and + PHPStan will again reason from a false premise. Worth a comment in review. +- `toString()` is cold (error paths and `__toString()` only), so the `array_map` + there is not a performance concern. Do not micro-optimize it back into an + `implode` over raw lookups. +- The `is_string($lookup)` narrowing added in step 5 is redundant at runtime + (`$lookupFilters` already guarantees it) but is the honest way to express the + invariant to the analyser. If `$lookupFilters` construction ever changes, that + guard is what keeps the call safe. +- Deferred: roughly 5800 further no-op `RenderContext::evaluate()` calls per + render pass remain, from filter arguments, conditions, and `ForTag`. Their + argument types do not prove them dead, so they need measurement rather than + deletion. Not attempted here. diff --git a/plans/README.md b/plans/README.md index 56dc80b..b7d72ac 100644 --- a/plans/README.md +++ b/plans/README.md @@ -15,6 +15,7 @@ and update your row when done. | 001 | Fast-path array scopes in `RenderContext::internalContextLookup()` | P1 | S | — | DONE | | 002 | Stop scanning every scope: lazy `RenderContext::iterateVariables()` | P1 | S | 001 (order only) | DONE | | 003 | Remove per-child overhead in `BodyNode` render/stream loops | P1 | S | 001, 002 (order only) | DONE | +| 004 | Drop the dead `evaluate()` call in `VariableLookup`, correct the `$lookups` type | P2 | S | 002 | TODO | Status values: TODO | IN PROGRESS | DONE | BLOCKED (with one-line reason) | REJECTED (with one-line rationale) From f0187a26833f1d95883b2408746ec813b24bef87 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 13:18:43 +0200 Subject: [PATCH 07/18] Fix variable lookup types and drop a dead evaluate call --- plans/README.md | 2 +- src/Nodes/VariableLookup.php | 26 ++++++++++++++++++++------ tests/Integration/VariableTest.php | 19 +++++++++++++++++++ 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/plans/README.md b/plans/README.md index b7d72ac..6b72ad9 100644 --- a/plans/README.md +++ b/plans/README.md @@ -15,7 +15,7 @@ and update your row when done. | 001 | Fast-path array scopes in `RenderContext::internalContextLookup()` | P1 | S | — | DONE | | 002 | Stop scanning every scope: lazy `RenderContext::iterateVariables()` | P1 | S | 001 (order only) | DONE | | 003 | Remove per-child overhead in `BodyNode` render/stream loops | P1 | S | 001, 002 (order only) | DONE | -| 004 | Drop the dead `evaluate()` call in `VariableLookup`, correct the `$lookups` type | P2 | S | 002 | TODO | +| 004 | Drop the dead `evaluate()` call in `VariableLookup`, correct the `$lookups` type | P2 | S | 002 | DONE | Status values: TODO | IN PROGRESS | DONE | BLOCKED (with one-line reason) | REJECTED (with one-line rationale) diff --git a/src/Nodes/VariableLookup.php b/src/Nodes/VariableLookup.php index f5c2e98..102ba58 100644 --- a/src/Nodes/VariableLookup.php +++ b/src/Nodes/VariableLookup.php @@ -6,10 +6,14 @@ use Keepsuit\Liquid\Contracts\HasParseTreeVisitorChildren; use Keepsuit\Liquid\Contracts\IsContextAware; use Keepsuit\Liquid\Exceptions\SyntaxException; +use Keepsuit\Liquid\Parse\ExpressionParser; use Keepsuit\Liquid\Render\RenderContext; use Keepsuit\Liquid\Support\MissingValue; use Keepsuit\Liquid\Support\UndefinedVariable; +/** + * @phpstan-import-type Expression from ExpressionParser + */ class VariableLookup implements CanBeEvaluated, HasParseTreeVisitorChildren { const FILTER_METHODS = ['size', 'first', 'last']; @@ -23,7 +27,7 @@ class VariableLookup implements CanBeEvaluated, HasParseTreeVisitorChildren public function __construct( public readonly string $name, - /** @var string[] */ + /** @var array */ public readonly array $lookups = [], ) { $lookupFilters = []; @@ -73,7 +77,19 @@ public function toString(): string return $this->name; } - return implode('.', [$this->name, ...$this->lookups]); + $lookups = array_map( + fn (mixed $lookup): string => match (true) { + is_string($lookup) => $lookup, + $lookup instanceof Literal => $lookup->value, + $lookup instanceof VariableLookup, $lookup instanceof RangeLookup => $lookup->toString(), + is_bool($lookup) => $lookup ? 'true' : 'false', + $lookup === null => '', + default => (string) $lookup, + }, + $this->lookups, + ); + + return implode('.', [$this->name, ...$lookups]); } public function __toString(): string @@ -88,9 +104,7 @@ public function parseTreeVisitorChildren(): array public function evaluate(RenderContext $context): mixed { - $name = $context->evaluate($this->name); - assert(is_string($name)); - $variables = $context->iterateVariables($name); + $variables = $context->iterateVariables($this->name); if ($this->lookups === []) { foreach ($variables as $variable) { @@ -114,7 +128,7 @@ public function evaluate(RenderContext $context): mixed $nextObject = $context->evaluate($context->internalContextLookup($object, $key)); - if ($nextObject instanceof MissingValue && is_iterable($object) && in_array($i, $this->lookupFilters, true)) { + if ($nextObject instanceof MissingValue && is_string($lookup) && is_iterable($object) && in_array($i, $this->lookupFilters, true)) { $nextObject = $context->applyFilter($lookup, $object); } diff --git a/tests/Integration/VariableTest.php b/tests/Integration/VariableTest.php index e53e419..1ad20db 100644 --- a/tests/Integration/VariableTest.php +++ b/tests/Integration/VariableTest.php @@ -1,5 +1,7 @@ setRethrowErrors(false) + ->setStrictVariables(true) + ->build(); + + $template = parseTemplate('{{ a[empty] }}', $environment); + $context = $environment->newRenderContext(); + + expect($template->render($context))->toBe(''); + + expect($template->getErrors()) + ->toHaveCount(1) + ->{0}->toBeInstanceOf(UndefinedVariableException::class) + ->{0}->getMessage()->toBe('Variable `a.empty` not found'); +}); + function generator(): Generator { yield '1'; From ec04bf2b42f4d739914eca39725f18b394e255e9 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 14:57:58 +0200 Subject: [PATCH 08/18] improved variable lookup --- src/Nodes/VariableLookup.php | 53 ++++++++---------------------- src/Parse/ExpressionParser.php | 16 +++++++-- tests/Integration/VariableTest.php | 14 ++------ 3 files changed, 29 insertions(+), 54 deletions(-) diff --git a/src/Nodes/VariableLookup.php b/src/Nodes/VariableLookup.php index 102ba58..6e8689d 100644 --- a/src/Nodes/VariableLookup.php +++ b/src/Nodes/VariableLookup.php @@ -6,38 +6,21 @@ use Keepsuit\Liquid\Contracts\HasParseTreeVisitorChildren; use Keepsuit\Liquid\Contracts\IsContextAware; use Keepsuit\Liquid\Exceptions\SyntaxException; -use Keepsuit\Liquid\Parse\ExpressionParser; use Keepsuit\Liquid\Render\RenderContext; use Keepsuit\Liquid\Support\MissingValue; use Keepsuit\Liquid\Support\UndefinedVariable; -/** - * @phpstan-import-type Expression from ExpressionParser - */ class VariableLookup implements CanBeEvaluated, HasParseTreeVisitorChildren { const FILTER_METHODS = ['size', 'first', 'last']; private const LOOKUP_REGEX = '{\.([\w\-]+)|\["([\w\-]+)"\]|\[\'([\w\-]+)\'\]|\[(\d+)\]}'; - /** - * @var int[] - */ - public readonly array $lookupFilters; - public function __construct( public readonly string $name, - /** @var array */ + /** @var array */ public readonly array $lookups = [], - ) { - $lookupFilters = []; - foreach ($this->lookups as $i => $lookup) { - if (in_array($lookup, self::FILTER_METHODS, true)) { - $lookupFilters[] = $i; - } - } - $this->lookupFilters = $lookupFilters; - } + ) {} /** * Parses `a.b[0]["c"]` into a name plus its lookups. @@ -77,19 +60,7 @@ public function toString(): string return $this->name; } - $lookups = array_map( - fn (mixed $lookup): string => match (true) { - is_string($lookup) => $lookup, - $lookup instanceof Literal => $lookup->value, - $lookup instanceof VariableLookup, $lookup instanceof RangeLookup => $lookup->toString(), - is_bool($lookup) => $lookup ? 'true' : 'false', - $lookup === null => '', - default => (string) $lookup, - }, - $this->lookups, - ); - - return implode('.', [$this->name, ...$lookups]); + return implode('.', [$this->name, ...$this->lookups]); } public function __toString(): string @@ -121,19 +92,21 @@ public function evaluate(RenderContext $context): mixed $object = iterator_to_array($object, preserve_keys: false); } - foreach ($this->lookups as $i => $lookup) { - $key = $context->evaluate($lookup) ?? ''; + foreach ($this->lookups as $lookup) { + $key = $lookup instanceof VariableLookup ? $context->evaluate($lookup) : $lookup; - assert(is_string($key) || is_int($key)); + if (! (is_string($key) || is_int($key))) { + continue 2; + } $nextObject = $context->evaluate($context->internalContextLookup($object, $key)); - if ($nextObject instanceof MissingValue && is_string($lookup) && is_iterable($object) && in_array($i, $this->lookupFilters, true)) { - $nextObject = $context->applyFilter($lookup, $object); - } - if ($nextObject instanceof MissingValue) { - continue 2; + if (is_iterable($object) && is_string($lookup) && in_array($lookup, self::FILTER_METHODS, true)) { + $nextObject = $context->applyFilter($lookup, $object); + } else { + continue 2; + } } $object = $nextObject; diff --git a/src/Parse/ExpressionParser.php b/src/Parse/ExpressionParser.php index f3816f0..43a113b 100644 --- a/src/Parse/ExpressionParser.php +++ b/src/Parse/ExpressionParser.php @@ -52,7 +52,7 @@ public function parseExpression(): mixed protected function parseVariable(): VariableLookup { $name = $this->tokenStream->consume(TokenType::Identifier)->data; - $lookups = $this->parseVariableLookups(); + $lookups = $this->parseVariableLookups($name); return new VariableLookup( name: $name, @@ -63,7 +63,7 @@ protected function parseVariable(): VariableLookup /** * @throws SyntaxException */ - protected function parseVariableLookups(): array + protected function parseVariableLookups(string $variableName): array { $lookups = []; @@ -74,7 +74,17 @@ protected function parseVariableLookups(): array continue; } if ($this->tokenStream->consumeOrFalse(TokenType::OpenSquare)) { - $lookups[] = $this->tokenStream->expression(); + $expression = $this->tokenStream->expression(); + $lookups[] = match (true) { + is_string($expression), is_int($expression), $expression instanceof VariableLookup => $expression, + default => throw new SyntaxException(sprintf('Invalid variable lookup: %s[%s]', $variableName, match (true) { + $expression instanceof Literal => $expression->value, + $expression instanceof RangeLookup => $expression->toString(), + is_bool($expression) => $expression ? 'true' : 'false', + $expression === null => 'nil', + default => (string) $expression + })), + }; $this->tokenStream->consume(TokenType::CloseSquare); continue; diff --git a/tests/Integration/VariableTest.php b/tests/Integration/VariableTest.php index 1ad20db..d1a080c 100644 --- a/tests/Integration/VariableTest.php +++ b/tests/Integration/VariableTest.php @@ -1,7 +1,6 @@ setRethrowErrors(false) ->setStrictVariables(true) ->build(); - $template = parseTemplate('{{ a[empty] }}', $environment); - $context = $environment->newRenderContext(); - - expect($template->render($context))->toBe(''); - - expect($template->getErrors()) - ->toHaveCount(1) - ->{0}->toBeInstanceOf(UndefinedVariableException::class) - ->{0}->getMessage()->toBe('Variable `a.empty` not found'); + expect(fn () => parseTemplate('{{ a[empty] }}', $environment)) + ->toThrow(\Keepsuit\Liquid\Exceptions\SyntaxException::class, 'Invalid variable lookup: a[empty]'); }); function generator(): Generator From 4f7c0366b664d6614e53c8df0f62e1ed0222febf Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 15:56:00 +0200 Subject: [PATCH 09/18] Optimize Drop property dispatch --- src/Drop.php | 44 +++++++++---------------- src/Support/DropMetadata.php | 59 ++++++++++++++++++++++++++++++++++ tests/Integration/DropTest.php | 29 +++++++++++++++-- tests/Stubs/CachableDrop.php | 15 +++++++++ 4 files changed, 116 insertions(+), 31 deletions(-) diff --git a/src/Drop.php b/src/Drop.php index cb0c644..987e35f 100644 --- a/src/Drop.php +++ b/src/Drop.php @@ -47,44 +47,30 @@ public function __toString(): string public function __get(string $name): mixed { - $invokableMethods = $this->getMetadata()->invokableMethods; - $cacheableMethods = $this->getMetadata()->cacheableMethods; - - $possibleNames = array_unique([ - $name, - Str::camel($name), - Str::snake($name), - ]); - - foreach ($possibleNames as $propertyName) { - if (in_array($propertyName, $this->getMetadata()->properties)) { - return $this->{$propertyName}; - } - } - - foreach ($possibleNames as $methodName) { - if (! in_array($methodName, $invokableMethods)) { - continue; - } + $metadata = $this->getMetadata(); + $resolution = $metadata->resolveStatic($name); - $isCacheable = in_array($methodName, $cacheableMethods); + if ($resolution !== null) { + $memberName = $resolution['name']; - if ($isCacheable && isset($this->cache[$methodName])) { - return $this->cache[$methodName]; + if ($resolution['type'] === 'property') { + return $this->{$memberName}; } - if (method_exists($this, $methodName)) { - $result = $this->{$methodName}(); + if ($resolution['cacheable'] && array_key_exists($memberName, $this->cache)) { + return $this->cache[$memberName]; + } - if ($isCacheable) { - $this->cache[$methodName] = $result; - } + $result = $this->{$memberName}(); - return $result; + if ($resolution['cacheable']) { + $this->cache[$memberName] = $result; } + + return $result; } - foreach ($possibleNames as $methodName) { + foreach ($metadata->possibleNames($name) as $methodName) { try { return $this->liquidMethodMissing($methodName); } catch (UndefinedDropMethodException) { diff --git a/src/Support/DropMetadata.php b/src/Support/DropMetadata.php index 302198f..2483723 100644 --- a/src/Support/DropMetadata.php +++ b/src/Support/DropMetadata.php @@ -23,6 +23,17 @@ final class DropMetadata protected static array $dropBaseMethods; + /** @var array> */ + private array $possibleNames = []; + + /** @var array + */ + private array $staticResolution = []; + public function __construct( /** @var list */ public readonly array $invokableMethods = [], @@ -34,6 +45,54 @@ public function __construct( public readonly array $dynamicProperties = [], ) {} + /** + * @return list + */ + public function possibleNames(string $name): array + { + return $this->possibleNames[$name] ??= array_values(array_unique([ + $name, + Str::camel($name), + Str::snake($name), + ])); + } + + /** + * @return array{type: 'property'|'method', name: string, cacheable: bool}|null + */ + public function resolveStatic(string $name): ?array + { + if (array_key_exists($name, $this->staticResolution)) { + return $this->staticResolution[$name]; + } + + $possibleNames = $this->possibleNames($name); + + foreach ($possibleNames as $propertyName) { + if (in_array($propertyName, $this->properties, true)) { + return $this->staticResolution[$name] = [ + 'type' => 'property', + 'name' => $propertyName, + 'cacheable' => false, + ]; + } + } + + foreach ($possibleNames as $methodName) { + if (in_array($methodName, $this->invokableMethods, true)) { + return $this->staticResolution[$name] = [ + 'type' => 'method', + 'name' => $methodName, + 'cacheable' => in_array($methodName, $this->cacheableMethods, true), + ]; + } + } + + $this->staticResolution[$name] = null; + + return null; + } + public static function init(Drop $drop): DropMetadata { if (isset(self::$cache[get_class($drop)])) { diff --git a/tests/Integration/DropTest.php b/tests/Integration/DropTest.php index 70ea8f4..6d8d283 100644 --- a/tests/Integration/DropTest.php +++ b/tests/Integration/DropTest.php @@ -144,8 +144,8 @@ ->properties->toBe([]); expect(invade(new CachableDrop)->getMetadata()) - ->invokableMethods->toBe(['notCached', 'cached']) - ->cacheableMethods->toBe(['cached']) + ->invokableMethods->toBe(['notCached', 'cached', 'cachedNull', 'cachedNullCalls']) + ->cacheableMethods->toBe(['cached', 'cachedNull']) ->properties->toBe([]); }); @@ -161,6 +161,31 @@ ->cached->toBe(0); }); +it('can cache null drop method results', function () { + $drop = new CachableDrop; + + expect($drop) + ->cachedNull->toBeNull() + ->cachedNull->toBeNull() + ->cachedNullCalls->toBe(1); +}); + +it('does not cache dynamic drop method results', function () { + $drop = new class extends \Keepsuit\Liquid\Drop + { + private int $calls = 0; + + protected function liquidMethodMissing(string $name): mixed + { + return sprintf('%s:%d', $name, ++$this->calls); + } + }; + + expect($drop) + ->unknownValue->toBe('unknownValue:1') + ->unknownValue->toBe('unknownValue:2'); +}); + it('can access drop data with snake and camel cases', function () { $drop = new ProductDrop; diff --git a/tests/Stubs/CachableDrop.php b/tests/Stubs/CachableDrop.php index 76a9788..e4f576e 100644 --- a/tests/Stubs/CachableDrop.php +++ b/tests/Stubs/CachableDrop.php @@ -11,6 +11,8 @@ class CachableDrop extends Drop protected int $cachedCounter = 0; + protected int $cachedNullCounter = 0; + public function notCached(): int { return $this->notCachedCounter++; @@ -21,4 +23,17 @@ public function cached(): int { return $this->cachedCounter++; } + + #[Cache] + public function cachedNull(): mixed + { + $this->cachedNullCounter++; + + return null; + } + + public function cachedNullCalls(): int + { + return $this->cachedNullCounter; + } } From b7aa180a64fc2b1414f0975f79f33a0bcf67d986 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 16:44:05 +0200 Subject: [PATCH 10/18] refactoring --- src/Drop.php | 19 ++++++++-------- src/Support/DropMetadata.php | 31 ++++++++++---------------- src/Support/DropStaticProperty.php | 25 +++++++++++++++++++++ src/Support/DropStaticPropertyType.php | 9 ++++++++ 4 files changed, 55 insertions(+), 29 deletions(-) create mode 100644 src/Support/DropStaticProperty.php create mode 100644 src/Support/DropStaticPropertyType.php diff --git a/src/Drop.php b/src/Drop.php index 987e35f..384feb5 100644 --- a/src/Drop.php +++ b/src/Drop.php @@ -6,6 +6,7 @@ use Keepsuit\Liquid\Contracts\IsContextAware; use Keepsuit\Liquid\Exceptions\UndefinedDropMethodException; use Keepsuit\Liquid\Support\DropMetadata; +use Keepsuit\Liquid\Support\DropStaticPropertyType; use Keepsuit\Liquid\Support\Str; class Drop implements IsContextAware @@ -48,23 +49,21 @@ public function __toString(): string public function __get(string $name): mixed { $metadata = $this->getMetadata(); - $resolution = $metadata->resolveStatic($name); + $resolution = $metadata->resolveStaticProperty($name); if ($resolution !== null) { - $memberName = $resolution['name']; - - if ($resolution['type'] === 'property') { - return $this->{$memberName}; + if ($resolution->type === DropStaticPropertyType::Property) { + return $this->{$resolution->name}; } - if ($resolution['cacheable'] && array_key_exists($memberName, $this->cache)) { - return $this->cache[$memberName]; + if ($resolution->cacheable && array_key_exists($resolution->name, $this->cache)) { + return $this->cache[$resolution->name]; } - $result = $this->{$memberName}(); + $result = $this->{$resolution->name}(); - if ($resolution['cacheable']) { - $this->cache[$memberName] = $result; + if ($resolution->cacheable) { + $this->cache[$resolution->name] = $result; } return $result; diff --git a/src/Support/DropMetadata.php b/src/Support/DropMetadata.php index 2483723..fad7b01 100644 --- a/src/Support/DropMetadata.php +++ b/src/Support/DropMetadata.php @@ -26,11 +26,7 @@ final class DropMetadata /** @var array> */ private array $possibleNames = []; - /** @var array + /** @var array */ private array $staticResolution = []; @@ -57,10 +53,7 @@ public function possibleNames(string $name): array ])); } - /** - * @return array{type: 'property'|'method', name: string, cacheable: bool}|null - */ - public function resolveStatic(string $name): ?array + public function resolveStaticProperty(string $name): ?DropStaticProperty { if (array_key_exists($name, $this->staticResolution)) { return $this->staticResolution[$name]; @@ -70,21 +63,21 @@ public function resolveStatic(string $name): ?array foreach ($possibleNames as $propertyName) { if (in_array($propertyName, $this->properties, true)) { - return $this->staticResolution[$name] = [ - 'type' => 'property', - 'name' => $propertyName, - 'cacheable' => false, - ]; + return $this->staticResolution[$name] = new DropStaticProperty( + name: $propertyName, + type: DropStaticPropertyType::Property, + cacheable: false + ); } } foreach ($possibleNames as $methodName) { if (in_array($methodName, $this->invokableMethods, true)) { - return $this->staticResolution[$name] = [ - 'type' => 'method', - 'name' => $methodName, - 'cacheable' => in_array($methodName, $this->cacheableMethods, true), - ]; + return $this->staticResolution[$name] = new DropStaticProperty( + name: $methodName, + type: DropStaticPropertyType::Method, + cacheable: in_array($methodName, $this->cacheableMethods, true) + ); } } diff --git a/src/Support/DropStaticProperty.php b/src/Support/DropStaticProperty.php new file mode 100644 index 0000000..dd765be --- /dev/null +++ b/src/Support/DropStaticProperty.php @@ -0,0 +1,25 @@ + Date: Wed, 29 Jul 2026 17:30:22 +0200 Subject: [PATCH 11/18] Generalize drop member resolution and caching - Replace static-property resolution with shared member resolution - Add coverage for cached method calls across drop instances --- src/Drop.php | 6 ++--- src/Support/DropMemberResolution.php | 25 +++++++++++++++++++ ...ticPropertyType.php => DropMemberType.php} | 2 +- src/Support/DropMetadata.php | 17 +++++-------- src/Support/DropStaticProperty.php | 25 ------------------- tests/Integration/DropTest.php | 3 +++ 6 files changed, 38 insertions(+), 40 deletions(-) create mode 100644 src/Support/DropMemberResolution.php rename src/Support/{DropStaticPropertyType.php => DropMemberType.php} (74%) delete mode 100644 src/Support/DropStaticProperty.php diff --git a/src/Drop.php b/src/Drop.php index 384feb5..90a33ff 100644 --- a/src/Drop.php +++ b/src/Drop.php @@ -5,8 +5,8 @@ use Keepsuit\Liquid\Concerns\ContextAware; use Keepsuit\Liquid\Contracts\IsContextAware; use Keepsuit\Liquid\Exceptions\UndefinedDropMethodException; +use Keepsuit\Liquid\Support\DropMemberType; use Keepsuit\Liquid\Support\DropMetadata; -use Keepsuit\Liquid\Support\DropStaticPropertyType; use Keepsuit\Liquid\Support\Str; class Drop implements IsContextAware @@ -49,10 +49,10 @@ public function __toString(): string public function __get(string $name): mixed { $metadata = $this->getMetadata(); - $resolution = $metadata->resolveStaticProperty($name); + $resolution = $metadata->resolveStaticMember($name); if ($resolution !== null) { - if ($resolution->type === DropStaticPropertyType::Property) { + if ($resolution->type === DropMemberType::Property) { return $this->{$resolution->name}; } diff --git a/src/Support/DropMemberResolution.php b/src/Support/DropMemberResolution.php new file mode 100644 index 0000000..6b678fe --- /dev/null +++ b/src/Support/DropMemberResolution.php @@ -0,0 +1,25 @@ +> */ private array $possibleNames = []; - /** @var array + /** @var array */ private array $staticResolution = []; @@ -53,7 +53,7 @@ public function possibleNames(string $name): array ])); } - public function resolveStaticProperty(string $name): ?DropStaticProperty + public function resolveStaticMember(string $name): ?DropMemberResolution { if (array_key_exists($name, $this->staticResolution)) { return $this->staticResolution[$name]; @@ -63,20 +63,15 @@ public function resolveStaticProperty(string $name): ?DropStaticProperty foreach ($possibleNames as $propertyName) { if (in_array($propertyName, $this->properties, true)) { - return $this->staticResolution[$name] = new DropStaticProperty( - name: $propertyName, - type: DropStaticPropertyType::Property, - cacheable: false - ); + return $this->staticResolution[$name] = DropMemberResolution::property($propertyName); } } foreach ($possibleNames as $methodName) { if (in_array($methodName, $this->invokableMethods, true)) { - return $this->staticResolution[$name] = new DropStaticProperty( - name: $methodName, - type: DropStaticPropertyType::Method, - cacheable: in_array($methodName, $this->cacheableMethods, true) + return $this->staticResolution[$name] = DropMemberResolution::method( + $methodName, + in_array($methodName, $this->cacheableMethods, true) ); } } diff --git a/src/Support/DropStaticProperty.php b/src/Support/DropStaticProperty.php deleted file mode 100644 index dd765be..0000000 --- a/src/Support/DropStaticProperty.php +++ /dev/null @@ -1,25 +0,0 @@ -notCached->toBe(0) @@ -158,6 +159,8 @@ expect($drop) ->cached->toBe(0) + ->cached->toBe(0) + ->and($anotherDrop) ->cached->toBe(0); }); From 9312956eb42e747c158f96fa68d3ea4d2817250d Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Wed, 29 Jul 2026 17:38:47 +0200 Subject: [PATCH 12/18] Optimize filter application and add benchmarks --- performance/benchmarks/FilterBench.php | 53 ++++++++++++++++++++++++++ src/Nodes/Variable.php | 16 ++++++-- 2 files changed, 66 insertions(+), 3 deletions(-) create mode 100644 performance/benchmarks/FilterBench.php diff --git a/performance/benchmarks/FilterBench.php b/performance/benchmarks/FilterBench.php new file mode 100644 index 0000000..38270a3 --- /dev/null +++ b/performance/benchmarks/FilterBench.php @@ -0,0 +1,53 @@ +environment = EnvironmentFactory::new()->build(); + $this->noArgumentsTemplate = $this->environment->parseString(str_repeat('{{ value | upcase | escape }}', 32)); + $this->argumentsTemplate = $this->environment->parseString(str_repeat('{{ value | append: suffix | replace: from, to }}', 32)); + } + + public function benchNoArguments(): void + { + $this->noArgumentsTemplate->render($this->context()); + } + + public function benchArguments(): void + { + $this->argumentsTemplate->render($this->context()); + } + + private function context(): \Keepsuit\Liquid\Render\RenderContext + { + return $this->environment->newRenderContext(staticData: [ + 'value' => 'example', + 'suffix' => '-suffix', + 'from' => 'example', + 'to' => 'value', + ]); + } +} diff --git a/src/Nodes/Variable.php b/src/Nodes/Variable.php index fe7c16e..9fd98ac 100644 --- a/src/Nodes/Variable.php +++ b/src/Nodes/Variable.php @@ -84,9 +84,19 @@ public function evaluate(RenderContext $context): mixed } foreach ($this->filters as [$filterName, $filterArgs, $filterNamedArgs]) { - $filterArgs = $this->evaluateFilterExpressions($context, $filterArgs ?? []); - $filterNamedArgs = $this->evaluateFilterExpressions($context, $filterNamedArgs ?? []); - $output = $context->applyFilter($filterName, $output, [...$filterArgs, ...$filterNamedArgs]); + if ($filterArgs === [] && $filterNamedArgs === []) { + $output = $context->applyFilter($filterName, $output); + + continue; + } + + $filterArgs = $this->evaluateFilterExpressions($context, $filterArgs); + + if ($filterNamedArgs !== []) { + $filterArgs = [...$filterArgs, ...$this->evaluateFilterExpressions($context, $filterNamedArgs)]; + } + + $output = $context->applyFilter($filterName, $output, $filterArgs); } return $output; From 0764f98a9ee405b23d3761b986b94915a934a553 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Thu, 30 Jul 2026 09:42:11 +0200 Subject: [PATCH 13/18] deleted plans --- plans/001-lookup-array-fast-path.md | 269 ---------- plans/002-lazy-variable-lookup.md | 426 ---------------- plans/003-bodynode-child-overhead.md | 445 ----------------- ...lelookup-dead-evaluate-and-lookup-types.md | 470 ------------------ plans/README.md | 93 ---- 5 files changed, 1703 deletions(-) delete mode 100644 plans/001-lookup-array-fast-path.md delete mode 100644 plans/002-lazy-variable-lookup.md delete mode 100644 plans/003-bodynode-child-overhead.md delete mode 100644 plans/004-variablelookup-dead-evaluate-and-lookup-types.md delete mode 100644 plans/README.md diff --git a/plans/001-lookup-array-fast-path.md b/plans/001-lookup-array-fast-path.md deleted file mode 100644 index 3d21452..0000000 --- a/plans/001-lookup-array-fast-path.md +++ /dev/null @@ -1,269 +0,0 @@ -# Plan 001: Fast-path array scopes in `RenderContext::internalContextLookup()` - -> **Executor instructions**: Follow this plan step by step. Run every -> verification command and confirm the expected result before moving to the -> next step. If anything in the "STOP conditions" section occurs, stop and -> report — do not improvise. When done, update the status row for this plan -> in `plans/README.md`. -> -> **Drift check (run first)**: `git diff --stat 4a1bdd6..HEAD -- src/Render/RenderContext.php` -> If that file changed since this plan was written, compare the "Current state" -> excerpt against the live code before proceeding; on a mismatch, treat it as a -> STOP condition. - -## Status - -- **Priority**: P1 -- **Effort**: S -- **Risk**: LOW -- **Depends on**: none -- **Category**: perf -- **Planned at**: commit `4a1bdd6`, 2026-07-29 - -## Why this matters - -`internalContextLookup()` is the innermost function of variable resolution: it -runs **8989 times per render pass** of the theme benchmark suite. Today every -one of those calls enters a `try`/`catch` block and walks a `match (true)` -chain whose *first* arm tests `$scope instanceof Drop` — even though the -overwhelming majority of scopes are plain PHP arrays (the render scopes, the -context data, and the static variables are all arrays). Hoisting the array case -out of the `match` and above the `try` measured **−12.5% on render and −11.3% -on stream** — the single largest win found in the render phase, from one hunk. - -Semantics do not change: the array arm produces exactly the same value, and the -"key absent" case produces the same `MissingValue` the `default` arm produced. - -## Current state - -- `src/Render/RenderContext.php` — the render-time variable context. The method - to change is `internalContextLookup()` at lines 221–236. - -Exact code as it exists today at `src/Render/RenderContext.php:221-236`: - -```php - public function internalContextLookup(mixed $scope, int|string $key): mixed - { - try { - $value = match (true) { - $scope instanceof Drop => $scope->{$key}, - is_array($scope) && array_key_exists($key, $scope) => $scope[$key], - is_object($scope) && $this->objectHasProperty($scope, (string) $key) => $scope->{$key}, - is_object($scope) && $this->objectHasStaticProperty($scope, (string) $key) => $scope::$$key, - default => $this->missingValue, - }; - } catch (UndefinedDropMethodException) { - return $this->missingValue; - } - - return $this->normalizeValue($value); - } -``` - -Facts you need about the surrounding code: - -- `$this->missingValue` is a private, pre-allocated `MissingValue` instance - (declared at `src/Render/RenderContext.php:65`, constructed at line 102). - Callers detect "not found" with `$value instanceof MissingValue`. -- `normalizeValue()` (same file) resolves `Closure` and `MapsToLiquid` values - and must still be applied to whatever the array branch returns. -- An array scope can never throw `UndefinedDropMethodException` — that exception - comes from `Drop::__get()` (`src/Drop.php`) and from `objectHasProperty()` - reaching a magic getter. So moving the array case outside the `try` is safe. - -Repo conventions: PHP 8.2+, strict PHPStan level 9 over `src/`, formatting by -Laravel Pint (`pint.json`). Match the existing style in this file — early -returns and guard clauses are already used throughout (see `normalizeValue()` -and `findVariables()` in the same file). - -## Commands you will need - -| Purpose | Command | Expected on success | -|---------|---------|---------------------| -| Tests | `vendor/bin/pest` | `Tests: 800 passed` (or more), 0 failures | -| Static analysis | `vendor/bin/phpstan analyse --no-progress` | `[OK] No errors` | -| Formatting | `vendor/bin/pint --test` | exit 0 (no files need fixing) | -| Benchmark | see "Benchmark procedure" below | render/stream both improve | - -## Benchmark procedure - -Run this from the repo root, after the code change and before committing: - -```bash -vendor/bin/phpbench run \ - --filter='benchRender|benchStream' \ - --progress=none \ - --warmup=1 \ - --retry-threshold=5 \ - --report=aggregate \ - --output=json > build/plan-001.json - -php tools/phpbench-compare.php build/base.json build/plan-001.json -``` - -`build/base.json` is the committed baseline for this repo (captured at -`4a1bdd6`). The compare tool only reports on benchmark names present in both -files, so filtering to two subjects is expected and fine. - -**Expected**: both `LiquidBench::benchRender` and `LiquidBench::benchStream` -show a positive ops/s delta, roughly **+10% to +14%** each. Benchmark noise on -a developer laptop is ±2–3%; if you see a delta smaller than +5%, re-run once -before drawing a conclusion. A *negative* delta on either subject is a STOP -condition. - -Do not commit `build/plan-001.json` — `build/` output other than the committed -`base.json` is scratch. Check `git status` before committing. - -## Scope - -**In scope** (the only files you should modify): -- `src/Render/RenderContext.php` -- `plans/README.md` (status row only) - -**Out of scope** (do NOT touch, even though they look related): -- `src/Render/RenderContext.php`'s `objectHasProperty()` and - `objectHasStaticProperty()` — they are cold in this benchmark (0 calls) and - changing them adds risk for no measured gain. -- `normalizeValue()` — an early return for non-objects was measured at exactly - zero delta and was deliberately rejected. Do not add it. -- `src/Drop.php`, `src/Nodes/VariableLookup.php`, `src/Nodes/BodyNode.php` — - covered by other plans or explicitly rejected. -- `build/base.json` — the committed baseline. Never regenerate or overwrite it. - -## Git workflow - -- Work on branch `perf/render-stream-optimizations`. Create it from `main` if it - does not exist yet: `git switch -c perf/render-stream-optimizations`. If it - already exists (a previous plan created it), just switch to it. -- One commit for this plan. Message style in this repo is plain sentence case, - no conventional-commit prefixes (see `git log --oneline`: "Optimize parser and - renderer hot paths", "Skip partial ParseContext on cache hit, tidy tag - parsing"). Use: `Fast-path array scopes in context lookup`. -- Do NOT push and do NOT open a PR. - -## Steps - -### Step 1: Hoist the array case above the try block - -In `src/Render/RenderContext.php`, replace the body of -`internalContextLookup()` so that arrays are handled first, and the `match` -keeps only the remaining arms: - -```php - public function internalContextLookup(mixed $scope, int|string $key): mixed - { - if (is_array($scope)) { - if (! array_key_exists($key, $scope)) { - return $this->missingValue; - } - - return $this->normalizeValue($scope[$key]); - } - - try { - $value = match (true) { - $scope instanceof Drop => $scope->{$key}, - is_object($scope) && $this->objectHasProperty($scope, (string) $key) => $scope->{$key}, - is_object($scope) && $this->objectHasStaticProperty($scope, (string) $key) => $scope::$$key, - default => $this->missingValue, - }; - } catch (UndefinedDropMethodException) { - return $this->missingValue; - } - - return $this->normalizeValue($value); - } -``` - -Note the removed `is_array($scope) && array_key_exists($key, $scope)` arm — it -is now unreachable and must be deleted, not left in place. - -**Verify**: `vendor/bin/pest` → `Tests: 800 passed` (or more), 0 failures. - -### Step 2: Confirm static analysis and formatting - -**Verify**: -- `vendor/bin/phpstan analyse --no-progress` → `[OK] No errors` -- `vendor/bin/pint --test` → exit 0 - -If Pint reports the file needs formatting, run `vendor/bin/pint src/Render/RenderContext.php` -and re-run the tests. - -### Step 3: Benchmark - -Run the "Benchmark procedure" above. - -**Verify**: `php tools/phpbench-compare.php build/base.json build/plan-001.json` -→ positive ops/s delta on both `LiquidBench::benchRender` and -`LiquidBench::benchStream`, expected around +10% to +14%. - -Record the two delta percentages — you will report them back. - -### Step 4: Commit - -```bash -git status # confirm only src/Render/RenderContext.php and plans/README.md changed -git add src/Render/RenderContext.php plans/README.md -git commit -m "Fast-path array scopes in context lookup" -``` - -**Verify**: `git show --stat HEAD` → exactly two files changed; `git status` → -clean except untracked `build/plan-001.json`. - -## Test plan - -No new tests are required: this is a behaviour-preserving refactor of an -internal method that is already covered end to end. The existing coverage that -exercises this path: - -- `tests/Integration/ContextTest.php` — scope resolution, nested scopes, - static data and registers. -- `tests/Integration/VariableTest.php` and `tests/Integration/DropTest.php` — - array scopes, drop scopes, and missing-variable behaviour. -- `tests/Integration/SelfDropTest.php` — the `self` fallback path, which - depends on `MissingValue` being returned for absent keys. - -If any of these fail, the array fast path is not behaviour-preserving — -that is a STOP condition, not something to patch around. - -**Verification**: `vendor/bin/pest` → all pass, same count as before the change. - -## Done criteria - -ALL must hold: - -- [ ] `vendor/bin/pest` exits 0 with the same test count as before the change -- [ ] `vendor/bin/phpstan analyse --no-progress` reports `[OK] No errors` -- [ ] `vendor/bin/pint --test` exits 0 -- [ ] `grep -n 'is_array($scope) && array_key_exists' src/Render/RenderContext.php` returns no matches -- [ ] `php tools/phpbench-compare.php build/base.json build/plan-001.json` shows a positive delta on both `benchRender` and `benchStream` -- [ ] `git show --stat HEAD` lists only `src/Render/RenderContext.php` and `plans/README.md` -- [ ] `plans/README.md` status row for 001 updated to DONE - -## STOP conditions - -Stop and report back (do not improvise) if: - -- The code at `src/Render/RenderContext.php:221-236` does not match the - "Current state" excerpt. -- Any test fails after the change. Do not adjust tests to fit — a failure here - means the fast path changed behaviour, which it must not. -- The benchmark shows a *negative* delta on either subject across two runs. -- You find yourself needing to modify any file outside the in-scope list. -- `build/base.json` does not exist or `tools/phpbench-compare.php` exits - non-zero for a reason other than a regression threshold. - -## Maintenance notes - -- The array branch bypasses the `try`/`catch`. That is only correct while array - access cannot throw `UndefinedDropMethodException`. If a future change makes - `$scope` an `ArrayAccess` object routed through this branch, the guard must be - revisited — `is_array()` deliberately excludes `ArrayAccess`, keep it that way. -- The branch must keep calling `normalizeValue()`; dropping it would silently - break `Closure` and `MapsToLiquid` values stored directly in a scope. A - reviewer should check for exactly that. -- Deferred out of this plan: the `objectHasProperty()` path still calls - `get_object_vars()` (allocating an array of every public property) on each - probe of a plain-object scope. It measured 0 calls in the theme benchmark, so - it was left alone; it would matter for an application that puts plain PHP - objects into the render context. diff --git a/plans/002-lazy-variable-lookup.md b/plans/002-lazy-variable-lookup.md deleted file mode 100644 index 0154c8a..0000000 --- a/plans/002-lazy-variable-lookup.md +++ /dev/null @@ -1,426 +0,0 @@ -# Plan 002: Stop scanning every scope — add a lazy `RenderContext::iterateVariables()` - -> **Executor instructions**: Follow this plan step by step. Run every -> verification command and confirm the expected result before moving to the -> next step. If anything in the "STOP conditions" section occurs, stop and -> report — do not improvise. When done, update the status row for this plan -> in `plans/README.md`. -> -> **Drift check (run first)**: `git diff --stat 4a1bdd6..HEAD -- src/Render/RenderContext.php src/Nodes/VariableLookup.php` -> Plan 001 (`plans/001-lookup-array-fast-path.md`) also edits -> `src/Render/RenderContext.php`, in `internalContextLookup()`. A diff limited -> to that method is expected. Any other change to the two methods quoted under -> "Current state" is a STOP condition. - -## Status - -- **Priority**: P1 -- **Effort**: S -- **Risk**: LOW -- **Depends on**: plans/001-lookup-array-fast-path.md (ordering only — not a hard dependency) -- **Category**: perf -- **Planned at**: commit `4a1bdd6`, 2026-07-29 - -## Why this matters - -`findVariables()` resolves a bare variable name against every scope. Today it -*always* probes all of them — every render scope, then the context data, then -the static variables — collects every match into an array, and returns it. -Instrumenting one render pass of the theme benchmark: **1862 calls performing -7394 scope probes**, while nearly every caller consumes only the first match. - -The extra matches are not dead weight in general: `VariableLookup::evaluate()` -falls back to the next candidate when a *lookup path* (`a.b.c`) misses on the -nearest one. But that fallback is rare, and for a bare `{{ foo }}` — which has -no lookups at all — the later probes can never be used. - -Making the scan lazy (a `Generator` that yields matches as it finds them) lets -both callers stop at the first usable value while keeping the fallback semantics -exactly intact. Measured on top of plan 001: **−7.6% render, −6.9% stream**. - -`findVariables()` stays in place with its current signature and behaviour, so -this is not a breaking change for anyone calling it from a custom tag or drop. - -## Current state - -Two files change. - -### `src/Render/RenderContext.php` — the scope scanner - -Exact code today at `src/Render/RenderContext.php:180-214`: - -```php - public function findVariables(string $key): array - { - $variables = []; - - // Check the variable in all scopes + env data + static variables - $scopeCount = count($this->scopes); - for ($index = 0; $index < $scopeCount + 2; $index++) { - $scope = match (true) { - $index < $scopeCount => $this->scopes[$index], - $index === $scopeCount => $this->data, - default => $this->sharedState->staticVariables, - }; - - $value = $this->internalContextLookup($scope, $key); - - if (! $value instanceof MissingValue) { - $variables[] = $value; - } - } - - // Inject the implicit self drop only when no value (including explicit null) was found. - // An explicit `self = nil` leaves [null] in $variables, so the fallback is skipped, - // correctly distinguishing defined-null from undefined. - if ($variables === [] && $key === 'self') { - return [$this->getSelfDrop()]; - } - - foreach ($variables as $variable) { - if ($variable instanceof IsContextAware) { - $variable->setContext($this); - } - } - - return $variables; - } -``` - -Three behaviours in there are load-bearing and must survive: - -1. **Scan order**: innermost scope first (`$this->scopes[0]` … `[n]`), then - `$this->data`, then `$this->sharedState->staticVariables`. -2. **The `self` fallback**: the implicit `SelfDrop` is injected *only* when - nothing at all was found. An explicit `self = nil` yields `[null]`, which is - not empty, so the fallback is correctly skipped. Preserve that distinction — - track "did we find anything" as a flag; do not test the yielded value for - `null`. -3. **`setContext()`**: every returned value implementing `IsContextAware` gets - the context injected before the caller sees it. - -### `src/Nodes/VariableLookup.php` — the main consumer - -Exact code today at `src/Nodes/VariableLookup.php:89-101` (the head of -`evaluate()`): - -```php - public function evaluate(RenderContext $context): mixed - { - $name = $context->evaluate($this->name); - assert(is_string($name)); - $variables = $context->findVariables($name); - - if ($this->lookups === []) { - if ($context->options->strictVariables && $variables === []) { - return new UndefinedVariable($this->toString()); - } - - return $variables[0] ?? null; - } -``` - -and the rest of `evaluate()` (lines 103–135), which iterates the candidates and -uses `continue 2` to fall through to the next one when a lookup misses: - -```php - foreach ($variables as $object) { - $object = $context->evaluate($object); - - if ($object instanceof \Generator) { - $object = iterator_to_array($object, preserve_keys: false); - } - - foreach ($this->lookups as $i => $lookup) { - // ... lookup resolution ... - - if ($nextObject instanceof MissingValue) { - continue 2; - } - // ... - } - - return $object; - } - - return $context->options->strictVariables ? new UndefinedVariable($this->toString()) : null; -``` - -That `foreach` works unchanged over a `Generator` — this is why the change is -small. - -### Other callers - -`src/Drops/SelfDrop.php:20` and `:27` also call `findVariables()`. They are -**not** in scope: `findVariables()` keeps working exactly as before. - -### Conventions - -PHP 8.2+, PHPStan level 9 over `src/` (so the new method needs a -`@return \Generator` docblock — the bare `\Generator` return type is not -enough at level 9), Laravel Pint formatting. `\Generator` is already used as a -return type across this codebase without a `use` import — see -`src/Nodes/BodyNode.php::stream()` and `src/Nodes/Variable.php::stream()`. -Match that: write `\Generator`, do not add an import. - -## Commands you will need - -| Purpose | Command | Expected on success | -|---------|---------|---------------------| -| Tests | `vendor/bin/pest` | `Tests: 800 passed` (or more), 0 failures | -| Static analysis | `vendor/bin/phpstan analyse --no-progress` | `[OK] No errors` | -| Formatting | `vendor/bin/pint --test` | exit 0 | -| Benchmark | see "Benchmark procedure" | render/stream both improve | - -## Benchmark procedure - -```bash -vendor/bin/phpbench run \ - --filter='benchRender|benchStream' \ - --progress=none \ - --warmup=1 \ - --retry-threshold=5 \ - --report=aggregate \ - --output=json > build/plan-002.json - -php tools/phpbench-compare.php build/base.json build/plan-002.json -``` - -`build/base.json` is the committed baseline at `4a1bdd6` — it is the baseline -for *all* plans, so this comparison shows the **cumulative** gain of plan 001 + -plan 002, expected around **+18% to +22%** on both subjects. To isolate this -plan's own contribution, compare against plan 001's result instead: -`php tools/phpbench-compare.php build/plan-001.json build/plan-002.json` -→ expected around **+6% to +9%** on both subjects. - -If `build/plan-001.json` does not exist (plan 001 was run by someone else, or in -a different working copy), skip the isolated comparison and report only the -cumulative one. - -Do not commit anything under `build/` other than the already-committed -`base.json`. - -## Scope - -**In scope** (the only files you should modify): -- `src/Render/RenderContext.php` -- `src/Nodes/VariableLookup.php` -- `tests/Integration/ContextTest.php` (add tests — see "Test plan") -- `plans/README.md` (status row only) - -**Out of scope** (do NOT touch, even though they look related): -- `src/Drops/SelfDrop.php` — it calls `findVariables()`, which keeps its exact - current signature and behaviour. Leave it alone. -- The `foreach ($variables as $object)` lookup-fallback loop in - `VariableLookup::evaluate()` (lines 103 onward) — it already works over a - `Generator` unchanged. Do not restructure it. -- `src/Render/RenderContext.php::internalContextLookup()` — that is plan 001. -- `build/base.json`. - -## Git workflow - -- Work on branch `perf/render-stream-optimizations` (created by plan 001; create - it from `main` with `git switch -c perf/render-stream-optimizations` if it - does not exist). -- One commit for this plan. Repo message style is plain sentence case, no - conventional-commit prefixes. Use: `Resolve variables lazily instead of scanning every scope`. -- Do NOT push and do NOT open a PR. - -## Steps - -### Step 1: Add `iterateVariables()` and re-express `findVariables()` on top of it - -In `src/Render/RenderContext.php`, replace the whole `findVariables()` method -with these two methods: - -```php - public function findVariables(string $key): array - { - return iterator_to_array($this->iterateVariables($key), preserve_keys: false); - } - - /** - * @return \Generator - */ - public function iterateVariables(string $key): \Generator - { - $found = false; - - // Check the variable in all scopes + env data + static variables - $scopeCount = count($this->scopes); - for ($index = 0; $index < $scopeCount + 2; $index++) { - $scope = match (true) { - $index < $scopeCount => $this->scopes[$index], - $index === $scopeCount => $this->data, - default => $this->sharedState->staticVariables, - }; - - $value = $this->internalContextLookup($scope, $key); - - if (! $value instanceof MissingValue) { - $found = true; - - if ($value instanceof IsContextAware) { - $value->setContext($this); - } - - yield $value; - } - } - - // Inject the implicit self drop only when no value (including explicit null) was found. - // An explicit `self = nil` yields null before this point, so $found is true and the - // fallback is skipped, correctly distinguishing defined-null from undefined. - if (! $found && $key === 'self') { - yield $this->getSelfDrop(); - } - } -``` - -Two differences from the original that are intentional: - -- `setContext()` now happens per value as it is yielded, rather than in a second - pass over the collected array. Consumers see an identical result. -- The `self` fallback is driven by the `$found` flag instead of - `$variables === []`, because a generator cannot inspect what it already - yielded. This preserves the defined-null-vs-undefined distinction. - -**Verify**: `vendor/bin/pest` → all pass. `findVariables()` is now a thin -wrapper, so every existing consumer must still be green at this point, before -any caller is switched over. - -### Step 2: Switch `VariableLookup::evaluate()` to the lazy path - -In `src/Nodes/VariableLookup.php`, change the head of `evaluate()` from the -"Current state" excerpt to: - -```php - public function evaluate(RenderContext $context): mixed - { - $name = $context->evaluate($this->name); - assert(is_string($name)); - $variables = $context->iterateVariables($name); - - if ($this->lookups === []) { - foreach ($variables as $variable) { - return $variable; - } - - return $context->options->strictVariables ? new UndefinedVariable($this->toString()) : null; - } -``` - -Leave everything from `foreach ($variables as $object) {` onward exactly as it -is — it consumes the generator correctly without modification. - -Why the `foreach`-then-`return` shape: it takes the first yielded value and -abandons the generator, so the remaining scopes are never probed. `null` is a -legitimate first value (an explicitly-null variable), which is why this cannot -be written as a null-coalescing expression. - -**Verify**: -- `vendor/bin/pest` → all pass, same count as before. -- `vendor/bin/phpstan analyse --no-progress` → `[OK] No errors`. -- `vendor/bin/pint --test` → exit 0. - -### Step 3: Add regression tests for the lazy path - -See "Test plan" below for the exact cases. - -**Verify**: `vendor/bin/pest --filter='lazy variable'` → the new tests pass. -Then `vendor/bin/pest` → full suite green with the new tests included. - -### Step 4: Benchmark - -Run the "Benchmark procedure" above. Record both delta percentages. - -**Verify**: positive ops/s delta on both `benchRender` and `benchStream` -versus `build/base.json`. - -### Step 5: Commit - -```bash -git status -git add src/Render/RenderContext.php src/Nodes/VariableLookup.php tests/Integration/ContextTest.php plans/README.md -git commit -m "Resolve variables lazily instead of scanning every scope" -``` - -**Verify**: `git show --stat HEAD` → exactly the four files above. - -## Test plan - -Add three tests to `tests/Integration/ContextTest.php`. Model them structurally -on the tests already in that file — Pest `test('...', function () { ... })` with -`expect(...)->toBe(...)`; read two neighbouring tests first and match their -setup style for building a `RenderContext`. - -The cases, each pinning one behaviour this change could plausibly break: - -1. **"lazy variable resolution returns the innermost scope"** — the same name - defined in an outer scope and in a pushed inner scope resolves to the inner - value. Guards scan order. -2. **"lazy variable resolution distinguishes defined-null from undefined"** — - a variable explicitly set to `null` resolves to `null` (not to a fallback, - and not to an `UndefinedVariable` under `strictVariables`), while a name that - was never set resolves to `null` normally and to `UndefinedVariable` under - `strictVariables`. Guards the `$found` flag. -3. **"lazy variable resolution falls back when a lookup path misses"** — with - `a` defined in two scopes, the inner one an array *without* key `b` and the - outer one an array *with* key `b`, `a.b` resolves to the outer value. Guards - the `continue 2` fallback that the generator must still support. - -Case 3 is the important one: it is the only reason `findVariables()` collected -multiple candidates in the first place, and it is the behaviour a naive -"return the first match" rewrite would silently destroy. If `tests/Integration/ContextTest.php` -already covers it, say so in your report and skip adding a duplicate. - -Also confirm the `self` fallback stays covered — `tests/Integration/SelfDropTest.php` -exercises it; it must pass untouched. - -**Verification**: `vendor/bin/pest` → all pass, including 2–3 new tests. - -## Done criteria - -ALL must hold: - -- [ ] `vendor/bin/pest` exits 0, with 2–3 more tests than before the change -- [ ] `vendor/bin/phpstan analyse --no-progress` reports `[OK] No errors` -- [ ] `vendor/bin/pint --test` exits 0 -- [ ] `grep -n 'findVariables' src/Nodes/VariableLookup.php` returns no matches -- [ ] `grep -n 'public function findVariables' src/Render/RenderContext.php` still returns a match (the method was kept for BC) -- [ ] `php tools/phpbench-compare.php build/base.json build/plan-002.json` shows a positive delta on both `benchRender` and `benchStream` -- [ ] `git show --stat HEAD` lists only the four in-scope files -- [ ] `plans/README.md` status row for 002 updated to DONE - -## STOP conditions - -Stop and report back (do not improvise) if: - -- The code at `src/Render/RenderContext.php:180-214` or - `src/Nodes/VariableLookup.php:89-101` does not match the "Current state" - excerpts (beyond plan 001's edit to `internalContextLookup()`). -- `tests/Integration/SelfDropTest.php` fails — that means the `self` fallback - distinction broke, which is the subtlest risk in this plan. -- Any existing test fails. Do not modify existing tests to make them pass. -- The benchmark shows a negative delta versus `build/base.json` on either - subject across two runs. -- You conclude the lookup-fallback loop needs restructuring to work with a - generator. It does not — if it seems to, something else is wrong. - -## Maintenance notes - -- `findVariables()` and `iterateVariables()` must not drift apart. If a future - change adds a scope source (a fourth place to look), it goes in - `iterateVariables()` only; `findVariables()` stays a one-line wrapper. -- The generator is consumed twice-shaped but never rewound. A caller that needs - the candidate list more than once must use `findVariables()`, not - `iterateVariables()` — generators are single-pass. A reviewer should check any - new consumer for that. -- `setContext()` is now called only on values that are actually reached. If some - code depended on the old eager behaviour — every candidate getting a context, - including ones never used — it would change. Nothing in this repo does; that - was verified by grepping the callers of `findVariables()` before writing this - plan. -- Deferred: `SelfDrop::__get()` and `__isset()` still call `findVariables()` - eagerly (`src/Drops/SelfDrop.php:20`, `:27`). They are cold in the theme - benchmark, so switching them was left out to keep this diff small. diff --git a/plans/003-bodynode-child-overhead.md b/plans/003-bodynode-child-overhead.md deleted file mode 100644 index 22f7e5b..0000000 --- a/plans/003-bodynode-child-overhead.md +++ /dev/null @@ -1,445 +0,0 @@ -# Plan 003: Remove per-child overhead in the `BodyNode` render and stream loops - -> **Executor instructions**: Follow this plan step by step. Run every -> verification command and confirm the expected result before moving to the -> next step. If anything in the "STOP conditions" section occurs, stop and -> report — do not improvise. When done, update the status row for this plan -> in `plans/README.md`. -> -> **Drift check (run first)**: `git diff --stat 4a1bdd6..HEAD -- src/Nodes/BodyNode.php src/Render/RenderContext.php` -> `src/Nodes/BodyNode.php` must be unchanged since `4a1bdd6` — plans 001 and 002 -> do not touch it. `src/Render/RenderContext.php` will show plan 001's and plan -> 002's edits; that is expected. Any change to `hasInterrupt()` or to -> `BodyNode` is a STOP condition. - -## Status - -- **Priority**: P1 -- **Effort**: S -- **Risk**: LOW -- **Depends on**: plans/001, plans/002 (ordering only — not a hard dependency) -- **Category**: perf -- **Planned at**: commit `4a1bdd6`, 2026-07-29 - -## Why this matters - -`BodyNode` is the loop that walks every node of a template. One render pass of -the theme benchmark runs it 361 times over **4384 child nodes**, so anything -paid per child is paid thousands of times. Four things are paid per child today -and do not need to be: - -1. **`streamChild()` allocates a `Generator` for every non-streamable node.** - Text nodes and most tags are not `CanBeStreamed`, so streaming a typical - template allocates one generator object per node just to yield a single - string, plus the `yield from` delegation on top. -2. **`$node instanceof Tag` fires a method call on every tag node.** - `Tag::ensureTagIsEnabled()` (`src/Tag.php`) starts with - `if (! $this instanceof Disableable) { return; }` — so for the vast majority - of tags the call exists only to return immediately. Testing `Disableable` - at the call site skips it. -3. **`renderChild()` is an indirection with no purpose** — it has no overrides - anywhere in `src/`, `tests/`, or `performance/` (verified by grep) and does - nothing but forward to `$node->render($context)`. -4. **`hasInterrupt()` calls `count()`** on an array that is empty in the common - case, once per child, where a `!== []` comparison suffices. - -Measured as a bundle on top of plans 001 and 002: **−5.9% render, −9.4% stream**. -The four items were measured together, not individually — item 1 is -stream-only, items 2–4 affect both loops. - -## Current state - -### `src/Nodes/BodyNode.php` — the node-walking loops - -Exact code today, `src/Nodes/BodyNode.php:45-127`: - -```php - /** - * @throws LiquidException - */ - public function render(RenderContext $context): string - { - $context->resourceLimits->incrementRenderScore(count($this->children)); - - $output = ''; - - foreach ($this->children as $node) { - try { - if ($node instanceof Tag) { - $node->ensureTagIsEnabled($context); - } - - $output .= $this->renderChild($context, $node); - } catch (UndefinedVariableException|UndefinedDropMethodException|UndefinedFilterException $exception) { - $context->handleError($exception, $node->lineNumber); - } catch (\Throwable $exception) { - $output .= $context->handleError($exception, $node->lineNumber); - } - - if ($context->hasInterrupt()) { - break; - } - } - - $context->resourceLimits->incrementWriteScore($output); - - return $output; - } - - /** - * @return \Generator - * - * @throws LiquidException - */ - public function stream(RenderContext $context): \Generator - { - $context->resourceLimits->incrementRenderScore(count($this->children)); - - foreach ($this->children as $node) { - try { - if ($node instanceof Tag) { - $node->ensureTagIsEnabled($context); - } - - foreach ($this->streamChild($context, $node) as $output) { - $context->resourceLimits->incrementWriteScore($output); - yield $output; - } - } catch (UndefinedVariableException|UndefinedDropMethodException|UndefinedFilterException $exception) { - $context->handleError($exception, $node->lineNumber); - } catch (\Throwable $exception) { - $output = $context->handleError($exception, $node->lineNumber); - $context->resourceLimits->incrementWriteScore($output); - yield $output; - } - - if ($context->hasInterrupt()) { - break; - } - } - } - - protected function renderChild(RenderContext $context, Node $node): string - { - return $node->render($context); - } - - /** - * @return \Generator - */ - public function streamChild(RenderContext $context, Node $node): \Generator - { - if ($node instanceof CanBeStreamed) { - yield from $node->stream($context); - - return; - } - - yield $node->render($context); - } -``` - -The file's current imports (`src/Nodes/BodyNode.php:5-11`): - -```php -use Keepsuit\Liquid\Contracts\CanBeStreamed; -use Keepsuit\Liquid\Exceptions\LiquidException; -use Keepsuit\Liquid\Exceptions\UndefinedDropMethodException; -use Keepsuit\Liquid\Exceptions\UndefinedFilterException; -use Keepsuit\Liquid\Exceptions\UndefinedVariableException; -use Keepsuit\Liquid\Render\RenderContext; -use Keepsuit\Liquid\Tag; -``` - -### `src/Tag.php` — why the `Disableable` check is equivalent - -```php - public function ensureTagIsEnabled(RenderContext $context): void - { - if (! $this instanceof Disableable) { - return; - } - - if (! $context->tagDisabled(static::tagName())) { - return; - } - - throw new TagDisabledException(static::tagName()); - } -``` - -The method is a no-op unless `$this instanceof Disableable`. `Disableable` lives -at `Keepsuit\Liquid\Contracts\Disableable`. - -### `src/Render/RenderContext.php` — the interrupt check - -```php - public function hasInterrupt(): bool - { - return count($this->interrupts) > 0; - } -``` - -`$this->interrupts` is declared as `protected array $interrupts = [];` in the -same file, so `!== []` is exactly equivalent to `count(...) > 0`. - -### Conventions - -PHP 8.2+, PHPStan level 9 over `src/`, Laravel Pint. Note the level-9 -constraint on step 2: `ensureTagIsEnabled()` is declared on `Tag`, not on -`Disableable`, so testing `Disableable` alone makes PHPStan reject the call. -The plan's code keeps both checks — `Disableable` first (it filters out almost -everything at zero cost), `Tag` second (it satisfies the analyser). - -## Commands you will need - -| Purpose | Command | Expected on success | -|---------|---------|---------------------| -| Tests | `vendor/bin/pest` | `Tests: 800 passed` (or more), 0 failures | -| Static analysis | `vendor/bin/phpstan analyse --no-progress` | `[OK] No errors` | -| Formatting | `vendor/bin/pint --test` | exit 0 | -| Benchmark | see "Benchmark procedure" | render/stream both improve | - -## Benchmark procedure - -```bash -vendor/bin/phpbench run \ - --filter='benchRender|benchStream' \ - --progress=none \ - --warmup=1 \ - --retry-threshold=5 \ - --report=aggregate \ - --output=json > build/plan-003.json - -php tools/phpbench-compare.php build/base.json build/plan-003.json -``` - -Versus `build/base.json` this is the **cumulative** result of all three plans: -expected around **+19% on `benchRender` and +26% on `benchStream`**. - -To isolate this plan: `php tools/phpbench-compare.php build/plan-002.json build/plan-003.json` -→ expected roughly **+5% render, +9% stream**. Skip the isolated comparison if -`build/plan-002.json` does not exist. - -Do not commit anything under `build/` other than the already-committed -`base.json`. - -## Scope - -**In scope** (the only files you should modify): -- `src/Nodes/BodyNode.php` -- `src/Render/RenderContext.php` (the `hasInterrupt()` method only) -- `plans/README.md` (status row only) - -**Out of scope** (do NOT touch, even though they look related): -- `src/Tag.php` — `ensureTagIsEnabled()` keeps its internal `Disableable` - guard. It is public API and is called from elsewhere; the call-site check is - an addition, not a replacement. -- **Deleting `renderChild()` or `streamChild()`** — they become unused, but - `streamChild()` is `public` and `renderChild()` is `protected`, so removing - either is a breaking change for any downstream subclass. Leave both in place; - see "Maintenance notes". -- The `try`/`catch` structure and the error-handling branches in both loops — - do not restructure them. -- `incrementRenderScore()` / `incrementWriteScore()` call sites and their - ordering — the stream loop must keep scoring each chunk before yielding it. -- `src/Render/RenderContext.php` beyond `hasInterrupt()`. - -## Git workflow - -- Work on branch `perf/render-stream-optimizations` (created by plan 001; create - it from `main` with `git switch -c perf/render-stream-optimizations` if it - does not exist). -- One commit for this plan. Repo message style is plain sentence case, no - conventional-commit prefixes. Use: `Trim per-child overhead in body rendering`. -- Do NOT push and do NOT open a PR. - -## Steps - -### Step 1: Inline the non-streamable child case in `stream()` - -In `src/Nodes/BodyNode.php::stream()`, replace: - -```php - foreach ($this->streamChild($context, $node) as $output) { - $context->resourceLimits->incrementWriteScore($output); - yield $output; - } -``` - -with: - -```php - if ($node instanceof CanBeStreamed) { - foreach ($node->stream($context) as $output) { - $context->resourceLimits->incrementWriteScore($output); - yield $output; - } - } else { - $output = $node->render($context); - $context->resourceLimits->incrementWriteScore($output); - yield $output; - } -``` - -This is the same dispatch `streamChild()` performed, minus the intermediate -generator. Both branches stay inside the existing `try` block, so error handling -is unchanged. - -**Verify**: `vendor/bin/pest --filter=Stream` → all stream tests pass -(`tests/Integration/StreamTest.php` covers chunk boundaries, which is exactly -what this step could break). - -### Step 2: Check `Disableable` at the call site in both loops - -In `src/Nodes/BodyNode.php`, in **both** `render()` and `stream()`, replace: - -```php - if ($node instanceof Tag) { - $node->ensureTagIsEnabled($context); - } -``` - -with: - -```php - if ($node instanceof Disableable && $node instanceof Tag) { - $node->ensureTagIsEnabled($context); - } -``` - -Keep that operand order: `Disableable` first is the point of the change, `Tag` -second is what keeps PHPStan level 9 happy about the method call. - -Add the import alongside the existing ones at the top of the file: - -```php -use Keepsuit\Liquid\Contracts\Disableable; -``` - -Keep `use Keepsuit\Liquid\Tag;` — it is still referenced by the second operand. - -**Verify**: -- `vendor/bin/phpstan analyse --no-progress` → `[OK] No errors` -- `vendor/bin/pest` → all pass. Disabled-tag behaviour is covered by the - existing suite; if a test about disabled tags fails here, the operand order or - the `Disableable` import is wrong. - -### Step 3: Drop the `renderChild()` indirection at the call site - -In `src/Nodes/BodyNode.php::render()`, replace: - -```php - $output .= $this->renderChild($context, $node); -``` - -with: - -```php - $output .= $node->render($context); -``` - -Leave the `renderChild()` method definition in the file — see "Out of scope". - -**Verify**: `vendor/bin/pest` → all pass. - -### Step 4: Simplify `hasInterrupt()` - -In `src/Render/RenderContext.php`: - -```php - public function hasInterrupt(): bool - { - return $this->interrupts !== []; - } -``` - -**Verify**: `vendor/bin/pest --filter='break|continue'` → interrupt-related -tests pass. Then `vendor/bin/pest` → full suite green. - -### Step 5: Formatting, benchmark, commit - -```bash -vendor/bin/pint --test -``` - -Then run the "Benchmark procedure" above and record both delta percentages. - -```bash -git status -git add src/Nodes/BodyNode.php src/Render/RenderContext.php plans/README.md -git commit -m "Trim per-child overhead in body rendering" -``` - -**Verify**: `git show --stat HEAD` → exactly three files. - -## Test plan - -No new tests are required — every step is behaviour-preserving and the existing -suite already covers each affected path: - -- `tests/Integration/StreamTest.php` — streaming chunk boundaries. Step 1 - changes how chunks are produced, so this is the file that would catch a - mistake there. Note that streaming semantics include *how many* chunks a - template yields (the tests assert `toHaveCount(2)` and per-index values), so - a merged or split chunk fails loudly. That is intended: step 1 must not change - chunk boundaries. -- `tests/Unit/Tags/` and `tests/Integration/Tags/` — disabled-tag behaviour for - step 2. -- `tests/Unit/ResourceLimitsTest.php` — render/write scoring, which step 1 - moves the call sites of. -- Break/continue interrupt tests for step 4. - -If you would like one extra guard, add a test to `tests/Integration/StreamTest.php` -asserting that a template mixing text, an `{% if %}` block, and an output tag -streams the same chunk sequence before and after — but only if you can write it -without changing any existing test. - -**Verification**: `vendor/bin/pest` → all pass, count unchanged (or +1). - -## Done criteria - -ALL must hold: - -- [ ] `vendor/bin/pest` exits 0 with the same test count as before (or +1) -- [ ] `vendor/bin/phpstan analyse --no-progress` reports `[OK] No errors` -- [ ] `vendor/bin/pint --test` exits 0 -- [ ] `grep -n 'streamChild($context' src/Nodes/BodyNode.php` returns no matches (the call is gone; the method definition remains) -- [ ] `grep -n 'renderChild($context' src/Nodes/BodyNode.php` returns no matches -- [ ] `grep -n 'count($this->interrupts)' src/Render/RenderContext.php` returns no matches -- [ ] `grep -n 'protected function renderChild' src/Nodes/BodyNode.php` still returns a match (kept for BC) -- [ ] `php tools/phpbench-compare.php build/base.json build/plan-003.json` shows a positive delta on both subjects -- [ ] `git show --stat HEAD` lists only the three in-scope files -- [ ] `plans/README.md` status row for 003 updated to DONE - -## STOP conditions - -Stop and report back (do not improvise) if: - -- `src/Nodes/BodyNode.php` differs from the "Current state" excerpt. -- Any test in `tests/Integration/StreamTest.php` fails — that means step 1 - changed chunk boundaries, which it must not. -- PHPStan reports an error about `ensureTagIsEnabled()` not existing. The fix is - the operand order given in step 2; if that does not resolve it, stop rather - than adding a baseline entry or a `@phpstan-ignore` comment. -- Any test fails and the only way you can see to make it pass is editing the - test. -- The benchmark shows a negative delta on either subject across two runs. - -## Maintenance notes - -- `renderChild()` and `streamChild()` are now dead code kept only for backward - compatibility with downstream subclasses. Neither has an override anywhere in - this repo. They are the right thing to delete in the next major release — flag - that to the maintainer rather than doing it here. -- Because `streamChild()` is no longer called, a subclass that overrode it to - customise streaming would silently stop taking effect. That is the one real - behavioural risk in this plan, and it is why deleting the method now (which - would fail loudly instead) is a defensible alternative the maintainer may - prefer. Raise it; do not decide it yourself. -- The `Disableable && Tag` check duplicates the guard inside - `Tag::ensureTagIsEnabled()`. If that method ever grows behaviour that must run - for non-`Disableable` tags, the call-site check has to come out again. -- Deferred: coalescing runs of adjacent `Text` nodes at parse time would cut the - per-child cost further for both loops, but it is a parser-phase change and was - out of scope for this render-phase audit. diff --git a/plans/004-variablelookup-dead-evaluate-and-lookup-types.md b/plans/004-variablelookup-dead-evaluate-and-lookup-types.md deleted file mode 100644 index 04588ed..0000000 --- a/plans/004-variablelookup-dead-evaluate-and-lookup-types.md +++ /dev/null @@ -1,470 +0,0 @@ -# Plan 004: Drop the dead `evaluate()` call in `VariableLookup` and correct the `$lookups` type - -> **Executor instructions**: Follow this plan step by step. Run every -> verification command and confirm the expected result before moving to the -> next step. If anything in the "STOP conditions" section occurs, stop and -> report — do not improvise. When done, update the status row for this plan -> in `plans/README.md`. -> -> **Drift check (run first)**: `git diff --stat 24a34bb..HEAD -- src/Nodes/VariableLookup.php` -> If that file changed since this plan was written, compare the "Current state" -> excerpts against the live code before proceeding; on a mismatch, treat it as a -> STOP condition. - -## Status - -- **Priority**: P2 -- **Effort**: S -- **Risk**: LOW -- **Depends on**: plans/002-lazy-variable-lookup.md (already DONE — this plan edits code that plan introduced) -- **Category**: perf + bug -- **Planned at**: commit `24a34bb`, 2026-07-29 - -## Why this matters - -Two things, both in `src/Nodes/VariableLookup.php`, discovered together because -one hid the other. - -**The performance half.** `RenderContext::evaluate()` is the highest-volume -function in the render path — 11145 calls per pass of the theme benchmark, of -which 9283 (83%) do nothing but one `instanceof` check and a return. -`evaluate($this->name)` accounts for 1862 of those, and it can *never* do -anything: `VariableLookup::$name` is declared `public readonly string`, a real -PHP type declaration the engine enforces, and a string is never -`CanBeEvaluated`. There is even an `assert(is_string($name))` on the next line -confirming the intent. Deleting the call measured **−3.2% render, −2.6% stream** -over three interleaved A/B rounds. - -**The correctness half.** `$lookups` is annotated `/** @var string[] */`, and -that is **wrong**. `ExpressionParser::parseVariableLookups()` -(`src/Parse/ExpressionParser.php:64-87`) pushes a plain string for a dot lookup -(`a.b`) but pushes `$this->tokenStream->expression()` for a bracket lookup -(`a[b]`) — which can be a `VariableLookup`, `RangeLookup`, `Literal`, int, -float, bool, or null. PHPStan has been reasoning from a false premise about this -property, and it hides a reachable crash: - -``` -{{ a[empty] }} with strictVariables: true - → InternalException (wrapping "Object of class Literal could not be converted to string") -``` - -instead of the `UndefinedVariableException: Variable 'a.empty' not found` that -`{{ a[b] }}` and `{{ a[(1..2)] }}` correctly produce. The cause is -`toString()` calling `implode()` over `$this->lookups`; `Literal` is a backed -enum with no `__toString()`, so the implode throws. `VariableLookup` and -`RangeLookup` happen to survive only because they define `__toString()`. - -Fixing the annotation makes PHPStan surface exactly two real problems, both of -which this plan fixes properly. - -## Current state - -Everything changes in one file: `src/Nodes/VariableLookup.php`. - -The class header and imports today (`src/Nodes/VariableLookup.php:1-16`): - -```php -lookups === []) { - return $this->name; - } - - return implode('.', [$this->name, ...$this->lookups]); - } -``` - -The head of `evaluate()` (`src/Nodes/VariableLookup.php:89-93`): - -```php - public function evaluate(RenderContext $context): mixed - { - $name = $context->evaluate($this->name); - assert(is_string($name)); - $variables = $context->iterateVariables($name); -``` - -The filter-fallback branch inside the lookup loop -(`src/Nodes/VariableLookup.php:110-119`): - -```php - foreach ($this->lookups as $i => $lookup) { - $key = $context->evaluate($lookup) ?? ''; - - assert(is_string($key) || is_int($key)); - - $nextObject = $context->evaluate($context->internalContextLookup($object, $key)); - - if ($nextObject instanceof MissingValue && is_iterable($object) && in_array($i, $this->lookupFilters, true)) { - $nextObject = $context->applyFilter($lookup, $object); - } -``` - -### Facts you must not get wrong - -- **`$context->evaluate($lookup)` on line 111 is NOT dead and must stay.** - It is what resolves the variable inside a bracket lookup like `{{ a[b] }}`. - Removing it breaks 7 tests. Only the `evaluate($this->name)` call on line 91 - is dead. -- `$lookupFilters` (built in the constructor) holds only the indices `$i` where - the lookup is one of the strings in `FILTER_METHODS`. So inside the - `in_array($i, $this->lookupFilters, true)` branch, `$lookup` is always a - string at runtime — PHPStan just cannot see it. Narrow it with an explicit - `is_string($lookup)` in the condition, which is honest rather than a - suppression. -- The `Expression` type alias is declared on `ExpressionParser` - (`src/Parse/ExpressionParser.php:11`) as: - `string|int|float|bool|Literal|VariableLookup|RangeLookup|null`. - Other files in this repo import it with - `@phpstan-import-type Expression from ExpressionParser` — see the class - docblock of `src/Nodes/Variable.php` and `src/Tags/ForTag.php` for the exact - convention to copy. -- `Literal` (`src/Nodes/Literal.php`) is a **backed enum** (`case Empty = 'empty';` - `case Blank = 'blank';`), so its string form is `$literal->value`. -- `toString()` is on the cold path only — it is called from the two - `strictVariables` error branches and from `__toString()`. Readability beats - micro-optimization there. - -### Conventions - -PHP 8.2+, PHPStan level 9 over `src/`, Laravel Pint. This repo's CI forbids -silencing analysis errors: **do not add `@phpstan-ignore` comments, baseline -entries, inline `@var` overrides, or type casts to make an error go away.** Fix -the underlying cause. `match (true)` chains are the idiom used across this -codebase for this kind of dispatch — see `Variable::debugLabel()` in -`src/Nodes/Variable.php:136-146` for a near-identical shape you should mirror. - -## Commands you will need - -| Purpose | Command | Expected on success | -|---------|---------|---------------------| -| Tests | `vendor/bin/pest` | `Tests: 806 passed` (or more), 0 failures | -| Static analysis | `vendor/bin/phpstan analyse --no-progress` | `[OK] No errors` | -| Formatting | `vendor/bin/pint --test` | exit 0 | -| Benchmark | see "Benchmark procedure" | render/stream both improve | - -## Benchmark procedure - -```bash -vendor/bin/phpbench run \ - --filter='benchRender|benchStream' \ - --progress=none \ - --warmup=1 \ - --retry-threshold=5 \ - --report=aggregate \ - --output=json > build/plan-004.json - -php tools/phpbench-compare.php build/base.json build/plan-004.json -``` - -**Important expectation-setting**: `build/base.json` was captured on CI -hardware, so the absolute percentages it produces locally are inflated and not -meaningful. What matters here is only that neither subject *regresses*. This -plan's own contribution measured **−3.2% render / −2.6% stream** in a controlled -same-machine A/B, which is close to the ±2% band the compare tool treats as -noise — so a local single-run comparison may well show this plan as neutral. -**That is an acceptable outcome and is NOT a STOP condition.** Only a clear -regression (worse than −5% on either subject, reproduced across two runs) is. - -Do not commit anything under `build/` other than the already-committed -`base.json`. - -## Scope - -**In scope** (the only files you should modify): -- `src/Nodes/VariableLookup.php` -- `tests/Integration/VariableTest.php` (add the regression test — see "Test plan") -- `plans/README.md` (status row only) - -**Out of scope** (do NOT touch, even though they look related): -- `$context->evaluate($lookup)` on line 111 — see "Facts you must not get - wrong". It is load-bearing. -- `$context->evaluate($object)` at the top of the `foreach ($variables ...)` - loop — scope values genuinely can be `CanBeEvaluated`. -- `src/Parse/ExpressionParser.php` — the parser is correct; it was the - annotation that was wrong. -- `src/Nodes/Variable.php`, `src/Nodes/RangeLookup.php`, `src/Nodes/Literal.php`. -- `src/Render/RenderContext.php::evaluate()` — the other ~5800 no-op calls come - from call sites whose types do not prove them dead. Not this plan. -- `phpstan-baseline.neon` — must not gain entries. -- `build/base.json`. - -## Git workflow - -- Work on the existing branch `perf/render-stream-optimizations`. Do NOT create - a branch and do NOT switch branches. -- One commit for this plan. Repo message style is plain sentence case, no - conventional-commit prefixes (see `git log --oneline`). Use: - `Fix variable lookup types and drop a dead evaluate call` -- Do NOT push and do NOT open a PR. A PR (#69) already exists for this branch; - pushing is the maintainer's call. - -## Steps - -### Step 1: Reproduce the bug first - -Confirm the defect exists before fixing it. Create a scratch file -`/tmp/repro-004.php` (outside the repo — do NOT add it to the repo): - -```php -build(); -$tpl = $env->parseString('{{ a[empty] }}'); -$ctx = new RenderContext( - options: new RenderContextOptions(strictVariables: true, rethrowErrors: true), - environment: $env, -); - -try { - $tpl->render($ctx); - echo "NO EXCEPTION\n"; -} catch (\Throwable $e) { - echo get_class($e).': '.$e->getMessage()."\n"; -} -``` - -**Verify**: `php /tmp/repro-004.php` → prints -`Keepsuit\Liquid\Exceptions\InternalException: Internal exception`. - -If it instead prints an `UndefinedVariableException`, the bug is already fixed — -STOP and report. - -### Step 2: Delete the dead `evaluate()` call - -In `evaluate()`, replace: - -```php - $name = $context->evaluate($this->name); - assert(is_string($name)); - $variables = $context->iterateVariables($name); -``` - -with: - -```php - $variables = $context->iterateVariables($this->name); -``` - -`$name` has no other use in the method — confirm that with -`grep -n '\$name' src/Nodes/VariableLookup.php` before deleting (the only other -hits should be `$this->name`). - -**Verify**: `vendor/bin/pest` → `Tests: 806 passed`, 0 failures. - -### Step 3: Correct the `$lookups` annotation - -Add the type import to the class docblock, immediately above -`class VariableLookup`: - -```php -/** - * @phpstan-import-type Expression from ExpressionParser - */ -class VariableLookup implements CanBeEvaluated, HasParseTreeVisitorChildren -``` - -and add the matching import alongside the existing `use` statements: - -```php -use Keepsuit\Liquid\Parse\ExpressionParser; -``` - -Then change the constructor annotation: - -```php - public function __construct( - public readonly string $name, - /** @var array */ - public readonly array $lookups = [], - ) { -``` - -**Verify**: `vendor/bin/phpstan analyse --no-progress` → **exactly 2 errors**, -both in `src/Nodes/VariableLookup.php`: -- one on the `implode` in `toString()` (`argument.type`) -- one on the `applyFilter` call in the lookup loop (`argument.type`) - -Those two are the real problems the wrong annotation was hiding; steps 4 and 5 -fix them. If you see a different number of errors, or errors in other files, -STOP and report. - -### Step 4: Make `toString()` handle every lookup type - -Replace `toString()` with: - -```php - public function toString(): string - { - if ($this->lookups === []) { - return $this->name; - } - - $lookups = array_map( - fn (mixed $lookup): string => match (true) { - is_string($lookup) => $lookup, - $lookup instanceof Literal => $lookup->value, - $lookup instanceof VariableLookup, $lookup instanceof RangeLookup => $lookup->toString(), - is_bool($lookup) => $lookup ? 'true' : 'false', - $lookup === null => '', - default => (string) $lookup, - }, - $this->lookups, - ); - - return implode('.', [$this->name, ...$lookups]); - } -``` - -`default` covers the remaining `int|float`. This mirrors the existing -`Variable::debugLabel()` idiom. - -**Verify**: `php /tmp/repro-004.php` → now prints -`Keepsuit\Liquid\Exceptions\UndefinedVariableException: Variable 'a.empty' not found` -(exact quoting of the variable name may differ — what matters is that it is an -`UndefinedVariableException` naming `a.empty`, not an `InternalException`). - -### Step 5: Narrow `$lookup` at the filter-fallback call - -In the lookup loop, change the condition: - -```php - if ($nextObject instanceof MissingValue && is_string($lookup) && is_iterable($object) && in_array($i, $this->lookupFilters, true)) { - $nextObject = $context->applyFilter($lookup, $object); - } -``` - -Keep `$nextObject instanceof MissingValue` as the first operand — it is the -cheapest and rarest test, and this is on the hot path. - -**Verify**: -- `vendor/bin/phpstan analyse --no-progress` → `[OK] No errors` -- `vendor/bin/pint --test` → exit 0 -- `vendor/bin/pest` → `Tests: 806 passed`, 0 failures - -### Step 6: Add the regression test - -See "Test plan". - -**Verify**: `vendor/bin/pest` → all pass, one more test than before. - -### Step 7: Benchmark and commit - -Run the "Benchmark procedure". Record both deltas, and remember that neutral is -an acceptable result here. - -```bash -git status -git add src/Nodes/VariableLookup.php tests/Integration/VariableTest.php plans/README.md -git commit -m "Fix variable lookup types and drop a dead evaluate call" -``` - -**Verify**: `git show --stat HEAD` → exactly three files. - -## Test plan - -Add one regression test to `tests/Integration/VariableTest.php`, pinning the bug -from step 1. Read two neighbouring tests in that file first and match their -structure and helper usage (the file uses Pest `test('...', function () { ... })` -with this repo's template helpers). - -The case: **a bracket lookup whose key is a `Literal`, under `strictVariables`, -reports an undefined variable rather than an internal error.** Concretely, -rendering `{{ a[empty] }}` with `strictVariables: true` must raise -`UndefinedVariableException`, not `InternalException`. - -If `tests/Integration/VariableTest.php` turns out not to be the natural home -(for example if bracket-lookup tests live in `tests/Integration/OutputTest.php` -or `tests/Integration/ContextTest.php`), put it wherever the existing -bracket-lookup tests are and say so in your report — matching the file's -neighbours matters more than the exact file named here. - -Do not add a test for the performance change; it is behaviour-preserving and -already covered. - -**Verification**: `vendor/bin/pest` → 807 passed (or more), 0 failures. - -## Done criteria - -ALL must hold: - -- [ ] `vendor/bin/pest` exits 0 with one more test than before (807+) -- [ ] `vendor/bin/phpstan analyse --no-progress` reports `[OK] No errors` -- [ ] `vendor/bin/pint --test` exits 0 -- [ ] `git diff HEAD~1 -- phpstan-baseline.neon` is empty (no new baseline entries) -- [ ] `grep -n 'assert(is_string($name))' src/Nodes/VariableLookup.php` returns no matches -- [ ] `grep -n '@var string\[\]' src/Nodes/VariableLookup.php` returns no matches -- [ ] `grep -n 'evaluate($lookup)' src/Nodes/VariableLookup.php` still returns a match (the load-bearing call was kept) -- [ ] `php /tmp/repro-004.php` prints an `UndefinedVariableException`, not an `InternalException` -- [ ] `php tools/phpbench-compare.php build/base.json build/plan-004.json` shows no clear regression on either subject -- [ ] `git show --stat HEAD` lists only the three in-scope files -- [ ] `plans/README.md` status row for 004 updated to DONE - -## STOP conditions - -Stop and report back (do not improvise) if: - -- The repro in step 1 does not reproduce. -- After step 3, PHPStan reports anything other than exactly the two expected - `argument.type` errors in `src/Nodes/VariableLookup.php`. -- You are tempted to add a `@phpstan-ignore` comment, a baseline entry, an - inline `@var`, or a cast to clear an analysis error. The repo forbids it — - stop instead. -- Removing the `evaluate($this->name)` call causes any test to fail. It should - not; if it does, the assumption that `$name` is always a `string` is false and - that changes the whole plan. -- You conclude that `$context->evaluate($lookup)` on line 111 should also be - removed. It must not be — that path is what 7 tests cover. -- The benchmark shows worse than −5% on either subject across two runs. - -## Maintenance notes - -- The `@var array` annotation is now the accurate contract. If - anyone "simplifies" it back to `string[]`, both bugs return silently and - PHPStan will again reason from a false premise. Worth a comment in review. -- `toString()` is cold (error paths and `__toString()` only), so the `array_map` - there is not a performance concern. Do not micro-optimize it back into an - `implode` over raw lookups. -- The `is_string($lookup)` narrowing added in step 5 is redundant at runtime - (`$lookupFilters` already guarantees it) but is the honest way to express the - invariant to the analyser. If `$lookupFilters` construction ever changes, that - guard is what keeps the call safe. -- Deferred: roughly 5800 further no-op `RenderContext::evaluate()` calls per - render pass remain, from filter arguments, conditions, and `ForTag`. Their - argument types do not prove them dead, so they need measurement rather than - deletion. Not attempted here. diff --git a/plans/README.md b/plans/README.md deleted file mode 100644 index 6b72ad9..0000000 --- a/plans/README.md +++ /dev/null @@ -1,93 +0,0 @@ -# Implementation Plans - -Generated by the improve skill on 2026-07-29 against commit `4a1bdd6`. Focus: -performance of the **render / streaming** phase only. Execute in the order -below — the measurements were taken by stacking them in this order, and each -plan's expected gain assumes the previous ones have landed. - -Each executor: read the plan fully before starting, honor its STOP conditions, -and update your row when done. - -## Execution order & status - -| Plan | Title | Priority | Effort | Depends on | Status | -|------|-------|----------|--------|------------|--------| -| 001 | Fast-path array scopes in `RenderContext::internalContextLookup()` | P1 | S | — | DONE | -| 002 | Stop scanning every scope: lazy `RenderContext::iterateVariables()` | P1 | S | 001 (order only) | DONE | -| 003 | Remove per-child overhead in `BodyNode` render/stream loops | P1 | S | 001, 002 (order only) | DONE | -| 004 | Drop the dead `evaluate()` call in `VariableLookup`, correct the `$lookups` type | P2 | S | 002 | DONE | - -Status values: TODO | IN PROGRESS | DONE | BLOCKED (with one-line reason) | REJECTED (with one-line rationale) - -## Dependency notes - -- No plan is technically blocked by another — they touch overlapping but - non-conflicting hunks. The ordering matters only because the measured deltas - below were taken cumulatively in this order, and because 002 and 003 both - touch `src/Nodes/BodyNode.php` / `src/Render/RenderContext.php`; running them - out of order means resolving trivial context drift by hand. - -## Measured baseline and expected outcome - -All numbers below were measured on a scratch copy of this repo at `4a1bdd6`, -PHP 8.5.9, opcache on, JIT off, using an interleaved A/B harness (15 runs per -side per round, minimum-of-run reported, 3+ rounds). "render"/"stream" are one -full pass of `ThemeRunner::render()` / `ThemeRunner::stream()` over the four -theme fixtures in `performance/tests/`. - -| Stage | render (µs, min) | stream (µs, min) | -|-------|-----------------:|-----------------:| -| baseline `4a1bdd6` | 4008 | 4312 | -| + plan 001 | 3506 (−12.5%) | 3823 (−11.3%) | -| + plan 002 | 3239 (−7.6%) | 3560 (−6.9%) | -| + plan 003 | 3047 (−5.9%) | 3226 (−9.4%) | - -Confirmed with the real benchmark harness (`vendor/bin/phpbench`, 10 revs × -10 its) against `build/base.json`: - -| Subject | Base | All three plans | Delta | -|---------|-----:|----------------:|------:| -| `LiquidBench::benchRender` | 208.4 ops/s | 249.5 ops/s | **+19.7%** | -| `LiquidBench::benchStream` | 190.3 ops/s | 240.8 ops/s | **+26.5%** | - -Peak memory is unchanged (6.13 MB both sides). The full test suite -(800 tests, 1990 assertions) and `phpstan analyse` at level 9 pass with all -three plans applied. - -## Call-count evidence - -Instrumented counters for one full `ThemeRunner::render()` pass at `4a1bdd6`: - -| Counter | Count | -|---------|------:| -| `BodyNode::render()` calls | 361 | -| child nodes rendered | 4384 | -| `RenderContext::findVariables()` calls | 1862 | -| scope probes performed by those calls | 7394 | -| `RenderContext::internalContextLookup()` calls | 8989 | -| `RenderContext::normalizeValue()` calls | 9046 | -| `RenderContext::set()` calls | 234 | -| `Drop::__get()` calls | 81 | - -## Findings considered and rejected - -- **Early-return for non-objects in `RenderContext::normalizeValue()`**: measured - as zero delta across three interleaved rounds. The existing - `is_object($value) && isset($this->sharedState->computedObjectsCache[$value])` - already short-circuits on the first check for scalars and arrays. -- **Per-class name-resolution cache in `Drop::__get()`** (memoize the - `Str::camel`/`Str::snake`/`array_unique`/`in_array` resolution per class): - correct in principle, but `Drop::__get()` is called only 81 times in the whole - theme benchmark, so it is unmeasurable here — the theme fixtures feed plain - arrays from `performance/Shopify/vision.database.yml`, not drops. Revisit only - after adding a drop-heavy benchmark fixture; without one there is no way to - verify the change helps. -- **`RenderContext::set()` / `Arr::set()` dotted-key parsing**: only 234 calls - per render pass. Not worth touching. -- **`ResourceLimits::incrementRenderScore()` per body**: 361 calls per pass. - Negligible. - -## Not audited - -Parse/tokenize phase, filter implementations, template caching, and the -`Profiler` — this run was scoped to render and streaming. From 1a7ae9133bd0c8bf9c0c06bdfa0993226a145ba6 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Thu, 30 Jul 2026 09:42:54 +0200 Subject: [PATCH 14/18] removed FilterBench --- performance/benchmarks/FilterBench.php | 53 -------------------------- 1 file changed, 53 deletions(-) delete mode 100644 performance/benchmarks/FilterBench.php diff --git a/performance/benchmarks/FilterBench.php b/performance/benchmarks/FilterBench.php deleted file mode 100644 index 38270a3..0000000 --- a/performance/benchmarks/FilterBench.php +++ /dev/null @@ -1,53 +0,0 @@ -environment = EnvironmentFactory::new()->build(); - $this->noArgumentsTemplate = $this->environment->parseString(str_repeat('{{ value | upcase | escape }}', 32)); - $this->argumentsTemplate = $this->environment->parseString(str_repeat('{{ value | append: suffix | replace: from, to }}', 32)); - } - - public function benchNoArguments(): void - { - $this->noArgumentsTemplate->render($this->context()); - } - - public function benchArguments(): void - { - $this->argumentsTemplate->render($this->context()); - } - - private function context(): \Keepsuit\Liquid\Render\RenderContext - { - return $this->environment->newRenderContext(staticData: [ - 'value' => 'example', - 'suffix' => '-suffix', - 'from' => 'example', - 'to' => 'value', - ]); - } -} From 972e4796b01a5cc5164b614f2e5e4fd076a8c176 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Thu, 30 Jul 2026 15:14:00 +0200 Subject: [PATCH 15/18] replace evaluate recursion with loop --- src/Render/RenderContext.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Render/RenderContext.php b/src/Render/RenderContext.php index 60b6a58..b43770e 100644 --- a/src/Render/RenderContext.php +++ b/src/Render/RenderContext.php @@ -144,8 +144,8 @@ public function stack(Closure $closure) public function evaluate(mixed $value): mixed { - if ($value instanceof CanBeEvaluated) { - return $this->evaluate($value->evaluate($this)); + while ($value instanceof CanBeEvaluated) { + $value = $value->evaluate($this); } return $value; From daa74c2ac6ee462a20e6741d1aed18e60ca2caad Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Thu, 30 Jul 2026 15:18:07 +0200 Subject: [PATCH 16/18] Text node optimization --- src/Nodes/BodyNode.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/Nodes/BodyNode.php b/src/Nodes/BodyNode.php index 1a16e19..3cfde82 100644 --- a/src/Nodes/BodyNode.php +++ b/src/Nodes/BodyNode.php @@ -53,6 +53,13 @@ public function render(RenderContext $context): string $output = ''; foreach ($this->children as $node) { + // Text is the majority of children and cannot fail or interrupt. + if ($node instanceof Text) { + $output .= $node->value; + + continue; + } + try { if ($node instanceof Disableable && $node instanceof Tag) { $node->ensureTagIsEnabled($context); @@ -85,6 +92,14 @@ public function stream(RenderContext $context): \Generator $context->resourceLimits->incrementRenderScore(count($this->children)); foreach ($this->children as $node) { + // Text is the majority of children and cannot fail or interrupt. + if ($node instanceof Text) { + $context->resourceLimits->incrementWriteScore($node->value); + yield $node->value; + + continue; + } + try { if ($node instanceof Disableable && $node instanceof Tag) { $node->ensureTagIsEnabled($context); From 17552c5bfcf71d3025bd3c8b40c92a9682f84248 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Thu, 30 Jul 2026 15:23:03 +0200 Subject: [PATCH 17/18] value normalization scalars --- src/Render/RenderContext.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Render/RenderContext.php b/src/Render/RenderContext.php index b43770e..a4010c5 100644 --- a/src/Render/RenderContext.php +++ b/src/Render/RenderContext.php @@ -246,7 +246,7 @@ public function internalContextLookup(mixed $scope, int|string $key): mixed return $this->missingValue; } - return $this->normalizeValue($value); + return is_object($value) ? $this->normalizeValue($value) : $value; } protected function objectHasProperty(object $object, string $property): bool @@ -278,11 +278,16 @@ protected function objectHasStaticProperty(object $object, string $property): bo public function normalizeValue(mixed $value): mixed { + // Only objects can need normalization, and scalars dominate the hot path. + if (! is_object($value)) { + return $value; + } + if ($value instanceof MissingValue) { return $value; } - if (is_object($value) && isset($this->sharedState->computedObjectsCache[$value])) { + if (isset($this->sharedState->computedObjectsCache[$value])) { return $this->sharedState->computedObjectsCache[$value]; } From 79300f0cadbdb360018ef58e1cf576778e576aae Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Thu, 30 Jul 2026 16:22:48 +0200 Subject: [PATCH 18/18] Optimize variable lookup and preserve outer-scope fallbacks - Avoid unnecessary scope allocations for scalar lookups - Add regression coverage for shadowed nested values --- src/Drops/SelfDrop.php | 9 ++-- src/Nodes/VariableLookup.php | 89 ++++++++++++++++++++++--------- src/Render/RenderContext.php | 84 +++++++++++++++++++---------- tests/Integration/ContextTest.php | 23 ++++++++ 4 files changed, 147 insertions(+), 58 deletions(-) diff --git a/src/Drops/SelfDrop.php b/src/Drops/SelfDrop.php index 52d938c..a356b8e 100644 --- a/src/Drops/SelfDrop.php +++ b/src/Drops/SelfDrop.php @@ -3,6 +3,7 @@ namespace Keepsuit\Liquid\Drops; use Keepsuit\Liquid\Render\RenderContext; +use Keepsuit\Liquid\Support\MissingValue; /** * Proxy object that resolves property lookups through the current render context scope chain. @@ -17,16 +18,16 @@ public function __construct( public function __get(string $name): mixed { - $variables = $this->context->findVariables($name); + $variable = $this->context->findVariable($name); - return $variables[0] ?? null; + return $variable instanceof MissingValue ? null : $variable; } public function __isset(string $name): bool { - $variables = $this->context->findVariables($name); + $variable = $this->context->findVariable($name); - return $variables !== []; + return ! $variable instanceof MissingValue; } public function __toString(): string diff --git a/src/Nodes/VariableLookup.php b/src/Nodes/VariableLookup.php index 6e8689d..9d4d6d3 100644 --- a/src/Nodes/VariableLookup.php +++ b/src/Nodes/VariableLookup.php @@ -75,49 +75,86 @@ public function parseTreeVisitorChildren(): array public function evaluate(RenderContext $context): mixed { - $variables = $context->iterateVariables($this->name); + $variable = $context->findVariable($this->name); + + if ($variable instanceof MissingValue) { + return $this->undefined($context); + } if ($this->lookups === []) { - foreach ($variables as $variable) { - return $variable; + return $variable; + } + + $result = $this->walkLookups($context, $variable); + + if (! $result instanceof MissingValue) { + return $result; + } + + // The name resolved but the lookup chain broke on the innermost value: an + // outer scope may still hold one the chain resolves against. + foreach ($context->findVariables($this->name) as $candidate) { + // Skip the value already walked above: re-walking it would repeat any + // side effects the broken chain triggered on the way. + if ($candidate === $variable) { + continue; } - return $context->options->strictVariables ? new UndefinedVariable($this->toString()) : null; + $result = $this->walkLookups($context, $candidate); + + if (! $result instanceof MissingValue) { + return $result; + } } - foreach ($variables as $object) { + return $this->undefined($context); + } + + protected function undefined(RenderContext $context): ?UndefinedVariable + { + return $context->options->strictVariables ? new UndefinedVariable($this->toString()) : null; + } + + /** + * Walks the lookup chain against $object, returning MissingValue if it breaks. + */ + protected function walkLookups(RenderContext $context, mixed $object): mixed + { + if ($object instanceof CanBeEvaluated) { $object = $context->evaluate($object); + } - if ($object instanceof \Generator) { - $object = iterator_to_array($object, preserve_keys: false); - } + if ($object instanceof \Generator) { + $object = iterator_to_array($object, preserve_keys: false); + } - foreach ($this->lookups as $lookup) { - $key = $lookup instanceof VariableLookup ? $context->evaluate($lookup) : $lookup; + foreach ($this->lookups as $lookup) { + $key = $lookup instanceof VariableLookup ? $context->evaluate($lookup) : $lookup; - if (! (is_string($key) || is_int($key))) { - continue 2; - } + if (! (is_string($key) || is_int($key))) { + return new MissingValue; + } - $nextObject = $context->evaluate($context->internalContextLookup($object, $key)); + $nextObject = $context->internalContextLookup($object, $key); - if ($nextObject instanceof MissingValue) { - if (is_iterable($object) && is_string($lookup) && in_array($lookup, self::FILTER_METHODS, true)) { - $nextObject = $context->applyFilter($lookup, $object); - } else { - continue 2; - } - } + if ($nextObject instanceof CanBeEvaluated) { + $nextObject = $context->evaluate($nextObject); + } - $object = $nextObject; - if ($object instanceof IsContextAware) { - $object->setContext($context); + if ($nextObject instanceof MissingValue) { + if (is_iterable($object) && is_string($lookup) && in_array($lookup, self::FILTER_METHODS, true)) { + $nextObject = $context->applyFilter($lookup, $object); + } else { + return $nextObject; } } - return $object; + $object = $nextObject; + if ($object instanceof IsContextAware) { + $object->setContext($context); + } } - return $context->options->strictVariables ? new UndefinedVariable($this->toString()) : null; + return $object; } } diff --git a/src/Render/RenderContext.php b/src/Render/RenderContext.php index a4010c5..e2bdac7 100644 --- a/src/Render/RenderContext.php +++ b/src/Render/RenderContext.php @@ -177,48 +177,76 @@ public function has(string $key): bool return $this->get($key) !== null; } - public function findVariables(string $key): array + /** + * Resolves $key against the scope chain and returns the innermost value. + * + * @return mixed the value, or MissingValue when the key is undefined everywhere + */ + public function findVariable(string $key): mixed { - return iterator_to_array($this->iterateVariables($key), preserve_keys: false); + // Deliberately not written as a loop over [...$this->scopes, $this->data, ...]: + // building that list would allocate an array on every variable reference. + foreach ($this->scopes as $scope) { + if (array_key_exists($key, $scope)) { + return $this->resolveVariable($scope[$key]); + } + } + + if (array_key_exists($key, $this->data)) { + return $this->resolveVariable($this->data[$key]); + } + + if (array_key_exists($key, $this->sharedState->staticVariables)) { + return $this->resolveVariable($this->sharedState->staticVariables[$key]); + } + + // Fall back to the implicit self drop only when no value was found anywhere. + return $key === 'self' ? $this->getSelfDrop() : $this->missingValue; } /** - * @return \Generator + * Every value $key resolves to, innermost scope first. + * + * Only useful to callers that need to fall back to an outer scope when the + * innermost value does not satisfy them; prefer findVariable() otherwise. + * + * @return list */ - public function iterateVariables(string $key): \Generator + public function findVariables(string $key): array { - $found = false; + $variables = []; - // Check the variable in all scopes + env data + static variables - $scopeCount = count($this->scopes); - for ($index = 0; $index < $scopeCount + 2; $index++) { - $scope = match (true) { - $index < $scopeCount => $this->scopes[$index], - $index === $scopeCount => $this->data, - default => $this->sharedState->staticVariables, - }; - - $value = $this->internalContextLookup($scope, $key); - - if ($value instanceof MissingValue) { - continue; + foreach ([...$this->scopes, $this->data, $this->sharedState->staticVariables] as $scope) { + if (array_key_exists($key, $scope)) { + $variables[] = $this->resolveVariable($scope[$key]); } + } - $found = true; + // Fall back to the implicit self drop only when no value was found anywhere. + if ($variables === [] && $key === 'self') { + return [$this->getSelfDrop()]; + } - if ($value instanceof IsContextAware) { - $value->setContext($this); - } + return $variables; + } - yield $value; + /** + * Normalizes a value pulled out of a scope and binds it to this context. + */ + protected function resolveVariable(mixed $value): mixed + { + // Only objects can need either step, and scalars dominate the hot path. + if (! is_object($value)) { + return $value; } - // Inject the implicit self drop only when no value (including explicit null) was found. - // An explicit `self = nil` yields null before this point, so $found is true and the - // fallback is skipped, correctly distinguishing defined-null from undefined. - if (! $found && $key === 'self') { - yield $this->getSelfDrop(); + $value = $this->normalizeValue($value); + + if ($value instanceof IsContextAware) { + $value->setContext($this); } + + return $value; } public function getSelfDrop(): SelfDrop diff --git a/tests/Integration/ContextTest.php b/tests/Integration/ContextTest.php index 35e4911..64aef6f 100644 --- a/tests/Integration/ContextTest.php +++ b/tests/Integration/ContextTest.php @@ -114,6 +114,29 @@ 'strict' => true, ]); +test('lookup falls back to outer scope when the inner value has no such key', function (bool $strict) { + $context = new RenderContext(options: new RenderContextOptions(strictVariables: $strict)); + $context->set('product', ['title' => 'outer']); + + $context->stack(function () use ($context, $strict) { + // The inner `product` shadows the outer one but cannot resolve `.title`, + // so resolution has to continue into the outer scope. + $context->set('product', ['handle' => 'inner']); + + expect($context->get('product.handle'))->toBe('inner'); + expect($context->get('product.title'))->toBe('outer'); + + if ($strict) { + expect($context->get('product.missing'))->toBeInstanceOf(UndefinedVariable::class); + } else { + expect($context->get('product.missing'))->toBeNull(); + } + }); +})->with([ + 'default' => false, + 'strict' => true, +]); + test('add item in inner scope', function (bool $strict) { $context = new RenderContext(options: new RenderContextOptions(strictVariables: $strict)); $context->stack(function () use ($context) {