fix(parser): allow dot-lookup property names to start with a digit#1196
Closed
SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
Closed
fix(parser): allow dot-lookup property names to start with a digit#1196SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
Conversation
Shopify objects and theme customization can define property names that start with a digit (e.g. `address.2country`). The parser grammar required every segment of a dot chain to match `identifier`, which itself requires `variableSegment = (letter | "_") ...`. Property names starting with a digit were treated as garbage text, and the language server's completion context was silently dropped when the cursor sat just after such a property. Introduce a dedicated `propertyName` rule that accepts leading digits and use it in `dotLookup` only. `variableSegment` stays restricted so variable declarations, filter names, and `for`/`capture`/named-argument targets still reject leading digits. Overrides added to the three `WithPlaceholder*` grammars so completion mode accepts the placeholder character in digit-starting property positions too. Closes Shopify#858
Contributor
|
Thanks for the contribution! In the near future with all themes moving to Liquid's new strict parser this will not be valid Liquid. The lookup will need to be |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR?
Fixes #858. The language server was silently dropping autocomplete after a property whose name started with a digit:
{{ address.2country.█ }} <- no completions offered for `name` or `code` {{ address.country.█ }} <- completions offeredWhat was the issue?
The parser grammar (
liquid-html.ohm) defined:Every dot-lookup segment was funnelled through
variableSegment, which only acceptsletteror_as the first character. Any property whose name started with a digit (e.g.2country) was treated as unparseable text.Downstream effect on completion:
createLiquidCompletionParamsappends the cursor placeholder█and callstoLiquidHtmlAST(..., { mode: 'completion' }). When that parse throws or falls through to the base-case fallback,completionContextis set toundefined, andObjectAttributeCompletionProviderreturns[]at its first guard (if (!params.completionContext) return []).The fix
Introduce a new
propertyNamerule that mirrorsidentifierbut allows leading digits, and use it indotLookuponly.variableSegmentstays restricted so variable declarations, filter names,for/capturetargets, and named arguments continue to reject leading digits.Matching overrides are added to the three
WithPlaceholder*grammars so that the completion-mode placeholder█is also accepted in property-name position:Why this is the right scope
Shopify actually supports property names that start with a digit — the issue's reproduction is a legitimate custom-object definition accepted by the platform. The grammar was incorrectly rejecting valid Liquid. Narrowing the fix to
dotLookup(rather than looseningidentifierorvariableSegment) preserves existing constraints where they matter:{% assign 2x = 1 %}) still fail — correct, Liquid forbids this{{ x | 2upcase }}) still fail — correctfor/captureloop variables still reject leading digits — correct.propertyNamein a dot chain accepts leading digits — the only place it shouldVerification
Minimal reproduction:
The completion provider now gets a valid
completionContextwith the cursor positioned inside a dot lookup on2country, so it walks the object graph normally and offers the nested properties.Tests
Added test cases in
stage-1-cst.spec.ts:address.2countryparses with lookups["2country"]address.2country.nameparses with lookups["2country", "name"]{{ address.2country.█ }}parses with lookups["2country", "█"]so completion providers can resolve the parent object and offer its propertiesRegression coverage
All 1,592 existing tests still pass across
liquid-html-parser(162),theme-check-common(856),prettier-plugin-liquid(141), andtheme-language-server-common(433). Zero regressions.Closes #858