From dbd550e1e51a851bbda1fbc672d24e13588c2f90 Mon Sep 17 00:00:00 2001 From: Colby McHenry Date: Fri, 30 Oct 2026 06:38:12 -0500 Subject: [PATCH] fix(liquid): a variable named with a hyphen is indexed under its whole name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Liquid allows hyphens in variable names (`{% assign form-field-label = … %}`, `{% capture icon-cart %}`), but the extractor read assign and capture names as \w+. PR #2814, which added capture, made such a name get no node rather than a cut-short one, so hyphenated variables were missing from search and codegraph_explore. Both declarations now read a name as a word character followed by word characters and hyphens. A capture's bare name is matched lazily up to whitespace or the tag's end, so in `{% capture title-%}` the hyphen still belongs to the -%} whitespace control and the name is `title`, while `{% capture title-text-%}` reads `title-text`. A name that goes on with a dot (`ghost.name`, `ghost-name.x`) still gets no node, in both tags. Liquid isn't extracted by the native kernel, so there is no kernel side. Validation: extraction.test.ts gains 5 capture forms (bare, tight -%}, trimmed, quoted, a {% liquid %} line) and an assign test (braced, trimmed with no spaces, a {% liquid %} line); they fail 6 of 6 with main's extractor and pass with the fix, and the cut-short guard now covers dotted names in both tags. A/B, each arm built from its own checkout, on PR #2814's theme set (Shopify/dawn 05222537, Shopify/horizon dfad90d8, Shopify/skeleton-theme a7a655e7, ChumsInc/chums-impulse-dev 4fd6f08e, agencecinq/cinquante-et-un ecb0035a, noclocks/shopify-devstore-theme e1747695, panoply/syncify 98d75fd0, Shopify/theme-tools bb33f49c) and controls expressjs/express 0dee2c52 and psf/requests 3c7baaa0: all ten byte-identical, as none of them uses a hyphenated name. On three themes that do (Zoront22/LOS 58dc2dfc, lucidnz/bootstrapify-1 b36ab7ff, ibarake/bittergracedc e36fa5db) the fix adds 4, 25 and 39 variable nodes, each with its one contains edge: exactly the 68 hyphenated declarations a separate scan finds, each checked to sit on a capture or assign tag naming it. Nothing is removed or retargeted, no unresolved ref changes, and file rows differ only in their node counts. Index time, median of six alternating runs: horizon 3.60 vs 3.64 s, bittergracedc 28.39 vs 28.24 s. Reimplements the part of PR #2504 by @Puma7 that PR #2814 left out. Refs #2498 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + __tests__/extraction.test.ts | 31 ++++++++++++++++++++++++++---- src/extraction/liquid-extractor.ts | 16 +++++++++------ 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10d18a2e4a..773a2459f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -183,6 +183,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - `codegraph sync` and the file watcher now link a Shopify section or snippet to the files that name it when it is added after them, or comes back after being deleted, whether they name it in a `{% render %}`, `{% include %}` or `{% section %}` tag or as a section `type` in a JSON template. Before, the link appeared only once a file naming it changed or the project was re-indexed, even in a repository with a single theme, so the new section or snippet looked unused and `codegraph affected` missed the files that use it. Re-index Shopify themes after upgrading to pick up links an earlier version missed. - A Shopify JSON template or section group saved by the theme editor now links the sections it lists. The editor writes a comment at the top of every file it saves, and a file with that comment, or with a trailing comma, both of which Shopify accepts, linked none of its sections, so in an editor-saved theme most sections looked unused. A layout's `{% sections 'header-group' %}` tag now links to the section group it renders, each section a JSON file lists is linked from the line that names it rather than from the top of the file, and a `{% schema %}` written inside `{% comment %}` or `{% raw %}` is no longer indexed as the section's own. Re-index Shopify themes after upgrading. Thanks @Puma7 for the report and the pull request. (#2494) - In Liquid, a variable declared with `{% capture %}`, like `{% capture title %}…{% endcapture %}`, a quoted `{% capture 'title' %}` or a `capture title` line inside `{% liquid %}`, is now indexed like one declared with `{% assign %}`, so search and `codegraph_explore` can find it. Before, only assigned variables were indexed, so a captured one could not be found, and `codegraph_explore` said its name was not in the index. Re-index Liquid projects after upgrading. Thanks @Puma7 for the report and the pull request. (#2498) +- In Liquid, a variable whose name has a hyphen in it, like `{% assign form-field-label = … %}` or `{% capture icon-cart %}`, is now indexed under its whole name, so search and `codegraph_explore` find it. Before, such a name was skipped, as Liquid allows hyphens in names but only letters, digits and underscores were read. A name that goes on with a dot still gets no node rather than a cut-short one, and in `{% capture title-%}` the hyphen still belongs to the tag's whitespace control. Re-index Liquid projects after upgrading. Thanks @Puma7. (#2498) - `codegraph sync` and the file watcher now link a route to the page or layout it loads lazily when that file is added after the router was indexed, comes back after being deleted, or gets the component the route renders in a later edit, whether or not the import names the file's extension: React Router's `lazy: () => import('./pages/Team')`, Vue Router's `component: () => import('@/views/Login')` and Angular's `loadComponent: () => import('./home/home.component')`. Before, the route stayed unlinked until the router file changed or the project was re-indexed, so the page looked unused and `codegraph affected` missed the tests that reach it through the router. Re-index after upgrading to pick up links an earlier version missed. - A file named the way Google names its tests, like `wire_format_unittest.cc` in protocolbuffers/protobuf, `logging_unittest.cc` in google/glog or a Python `foo_unittest.py`, now counts as a test, as `wire_format_test.cc` already did. Before, it was taken for production code: search and `codegraph_explore` ranked it alongside the code it tests, `codegraph affected` never listed it, and calls in production code could link to a class or type a unittest declares for itself, as google/breakpad's `stack_frame_entries_.size()` did to a unittest's `StackHelper`. Re-index projects that have such files after upgrading. - A file in a `unittests/` folder, where llvm/llvm-project and google/breakpad keep their unit tests, or in a folder named like a unittest, such as `dcheck_unittest/` in google/glog, now counts as a test whatever its own name says, like LLVM's `CountCopyAndMove.h` helper. Before, such a file was taken for production code unless its name said test, and none of them were kept apart from the program the way a `tests/` folder is: search and `codegraph_explore` ranked them alongside production code, `codegraph affected` missed them, and production code linked to a unit test's own declarations, as LLVM's `Ctx` and `OpInfo` variables did to ones its unit tests declare. A folder named just `unittest/`, like CPython's `Lib/unittest/`, is still production code. Re-index projects that have such folders after upgrading. diff --git a/__tests__/extraction.test.ts b/__tests__/extraction.test.ts index 5d3a5f5476..81c9c2e5ba 100644 --- a/__tests__/extraction.test.ts +++ b/__tests__/extraction.test.ts @@ -5337,6 +5337,12 @@ import 'package:flutter/material.dart'; ['{% liquid\n capture bare\n echo "Hi"\n endcapture\n%}', 'bare', 2, 2], ['{% liquid\r\n capture crlf # a note\r\n endcapture\r\n%}', 'crlf', 2, 2], ["{%- liquid\n capture 'quoted_line'\n endcapture\n-%}", 'quoted_line', 2, 2], + // A name with `-` in it is one name; the `-` of `-%}` trims whitespace. + ['{% capture title-text %}Hi{% endcapture %}', 'title-text', 1, 0], + ['{% capture title-text-%}Hi{% endcapture %}', 'title-text', 1, 0], + ['{%- capture title-text -%}Hi{%- endcapture -%}', 'title-text', 1, 0], + ["{% capture 'hero-alt' %}Hi{% endcapture %}", 'hero-alt', 1, 0], + ['{% liquid\n capture hero-alt\n endcapture\n%}', 'hero-alt', 2, 2], ] as const)('reads a capture declaration from %j', (code, name, line, column) => { const result = extractFromSource('sections/card.liquid', code); const variables = result.nodes.filter((n) => n.kind === 'variable'); @@ -5360,10 +5366,27 @@ import 'package:flutter/material.dart'; expect(result.nodes.filter((n) => n.kind === 'variable').map((n) => n.name)).toEqual(['text']); }); - it('never cuts a capture name short', () => { - // Liquid allows `-` and `.` in a name, which the assign rule doesn't read - // either; such a name gets no node rather than a wrong one. - const code = ['{% capture ghost-name %}x{% endcapture %}', '{% capture ghost.name %}x{% endcapture %}'].join('\n'); + it('reads an assign name with `-` in it whole', () => { + const code = [ + "{% assign title-text = 'Sale' %}", + '{%- assign page-title=page.title -%}', + '{% liquid', + ' assign hero-image = section.settings.image', + '%}', + ].join('\n'); + const variables = extractFromSource('sections/card.liquid', code).nodes.filter((n) => n.kind === 'variable'); + expect(variables.map((n) => [n.name, n.startLine])).toEqual([['title-text', 1], ['page-title', 2], ['hero-image', 4]]); + }); + + it('never cuts a name short', () => { + // Liquid allows `.` in a name too, which neither rule reads; such a name + // gets no node rather than a wrong one. + const code = [ + '{% capture ghost.name %}x{% endcapture %}', + '{% capture ghost-name.x %}x{% endcapture %}', + '{% assign ghost.name = 1 %}', + '{% assign ghost-name.x = 1 %}', + ].join('\n'); expect(extractFromSource('sections/card.liquid', code).nodes.filter((n) => n.kind === 'variable')).toEqual([]); }); diff --git a/src/extraction/liquid-extractor.ts b/src/extraction/liquid-extractor.ts index 4de1826c83..3536ec64e9 100644 --- a/src/extraction/liquid-extractor.ts +++ b/src/extraction/liquid-extractor.ts @@ -464,16 +464,20 @@ export class LiquidExtractor { * declarations as variables */ private extractAssignments(fileNodeId: string): void { + // A name is word characters and `-`, as Liquid reads it: `title-text` is one + // name. One that goes on with `.` gets no node rather than a cut-short one. + const name = `\\w[\\w-]*`; // Both spellings. Most of a modern theme's assigns are the bare kind. - for (const match of this.findTagOccurrences('assign', `(\\w+)\\s*=`)) { + for (const match of this.findTagOccurrences('assign', `(${name})\\s*=`)) { this.addVariable(fileNodeId, match.groups[1]!, match); } // A capture declares its name as an assign does, and Liquid reads a quoted - // name (`{% capture 'title' %}`) as the name inside the quotes. A name that - // goes on with `-` or `.`, which the assign rule doesn't read either, gets - // no node rather than a cut-short one. Captures come after assigns, so an - // assign keeps its id when a capture of the same name shares its line. - for (const match of this.findTagOccurrences('capture', `(?:'(\\w+)'|"(\\w+)"|(\\w+))(?=\\s|-?%\\}|$)`)) { + // name (`{% capture 'title' %}`) as the name inside the quotes. A bare name + // ends at the first whitespace or at the tag's end, `-%}` included: in + // `{% capture title-%}` the `-` trims whitespace, so the name is `title`. + // Captures come after assigns, so an assign keeps its id when a capture of + // the same name shares its line. + for (const match of this.findTagOccurrences('capture', `(?:'(${name})'|"(${name})"|(${name}?))(?=\\s|-?%\\}|$)`)) { const [, singleQuoted, doubleQuoted, bare] = match.groups; this.addVariable(fileNodeId, (singleQuoted ?? doubleQuoted ?? bare)!, match); }