From 73b63abbbb051016229f793f9d869ca708eb6e0b Mon Sep 17 00:00:00 2001 From: James Meng Date: Mon, 16 Mar 2026 16:05:38 -0700 Subject: [PATCH 1/4] =?UTF-8?q?Add=20go-to-definition=20for=20variable=20r?= =?UTF-8?q?eferences=20=E2=86=92=20assign=20tags?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user Cmd+Clicks on a variable like {{ my_var }}, the LSP now jumps to the {% assign my_var = ... %} that defines it. - New VariableDefinitionProvider handles VariableLookup → AssignMarkup - Shadowing works: later assigns win for later references - Global/contextual variables return null (no false positives) - 9 new tests, all 13 definition tests pass This covers assign only. Capture, for, tablerow, and @param are planned as follow-up tasks. Co-authored-by: Claude --- .../src/definitions/DefinitionProvider.ts | 2 + .../VariableDefinitionProvider.spec.ts | 181 ++++++++++++++++++ .../providers/VariableDefinitionProvider.ts | 110 +++++++++++ 3 files changed, 293 insertions(+) create mode 100644 packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts create mode 100644 packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts diff --git a/packages/theme-language-server-common/src/definitions/DefinitionProvider.ts b/packages/theme-language-server-common/src/definitions/DefinitionProvider.ts index 561d81fbc..94a08f37e 100644 --- a/packages/theme-language-server-common/src/definitions/DefinitionProvider.ts +++ b/packages/theme-language-server-common/src/definitions/DefinitionProvider.ts @@ -5,6 +5,7 @@ import { AugmentedJsonSourceCode, DocumentManager } from '../documents'; import { BaseDefinitionProvider } from './BaseDefinitionProvider'; import { SchemaTranslationStringDefinitionProvider } from './providers/SchemaTranslationStringDefinitionProvider'; import { TranslationStringDefinitionProvider } from './providers/TranslationStringDefinitionProvider'; +import { VariableDefinitionProvider } from './providers/VariableDefinitionProvider'; export class DefinitionProvider { private providers: BaseDefinitionProvider[]; @@ -20,6 +21,7 @@ export class DefinitionProvider { documentManager, getDefaultSchemaLocaleSourceCode, ), + new VariableDefinitionProvider(documentManager), ]; } diff --git a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts new file mode 100644 index 000000000..bf88125f2 --- /dev/null +++ b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts @@ -0,0 +1,181 @@ +import { assert, beforeEach, describe, expect, it } from 'vitest'; +import { DefinitionParams, LocationLink } from 'vscode-languageserver-protocol'; +import { DocumentManager } from '../../documents'; +import { DefinitionProvider } from '../DefinitionProvider'; + +describe('Module: VariableDefinitionProvider', () => { + let provider: DefinitionProvider; + let documentManager: DocumentManager; + + beforeEach(() => { + documentManager = new DocumentManager(); + provider = new DefinitionProvider( + documentManager, + async () => null, + async () => null, + ); + }); + + it('finds the definition of an assigned variable', async () => { + const source = '{% assign x = 1 %}{{ x }}'; + // 0123456789012345678901234 + // assign "x" starts at pos 10 (the "x" in assign markup) + // VariableLookup "x" starts at pos 22 + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 0, character: 22 }, // cursor on {{ x }} + }; + + const result = await provider.definitions(params); + + assert(result); + expect(result).toHaveLength(1); + assert(LocationLink.is(result[0])); + expect(result[0].targetUri).toBe('file:///test.liquid'); + // The target should point to the variable name in the assign tag + expect(result[0].targetRange).toEqual(result[0].targetSelectionRange); + }); + + it('handles variable shadowing — each reference jumps to the correct assign', async () => { + const source = ['{% assign x = 1 %}', '{{ x }}', '{% assign x = 2 %}', '{{ x }}'].join('\n'); + documentManager.open('file:///test.liquid', source, 1); + + // First {{ x }} on line 1 — should jump to first assign (line 0) + // character 4 = end of 'x' (offset lands inside VariableLookup range) + const params1: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 1, character: 4 }, + }; + const result1 = await provider.definitions(params1); + assert(result1); + expect(result1).toHaveLength(1); + assert(LocationLink.is(result1[0])); + expect(result1[0].targetRange.start.line).toBe(0); + + // Second {{ x }} on line 3 — should jump to second assign (line 2) + const params2: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 3, character: 4 }, + }; + const result2 = await provider.definitions(params2); + assert(result2); + expect(result2).toHaveLength(1); + assert(LocationLink.is(result2[0])); + expect(result2[0].targetRange.start.line).toBe(2); + }); + + it('returns null for global/contextual variables (no assign exists)', async () => { + const source = '{{ product }}'; + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 0, character: 5 }, + }; + + const result = await provider.definitions(params); + assert(result === null); + }); + + it('returns null for unknown variables with no definition', async () => { + const source = '{{ unknown }}'; + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 0, character: 5 }, + }; + + const result = await provider.definitions(params); + assert(result === null); + }); + + it('returns null for VariableLookup with null name (global access)', async () => { + // {{ ['product'] }} has a VariableLookup with name = null + const source = "{{ ['product'] }}"; + documentManager.open('file:///test.liquid', source, 1); + + // Offset 14 hits the VariableLookup(name=null) at position {3,14} + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 0, character: 14 }, + }; + + const result = await provider.definitions(params); + assert(result === null); + }); + + it('returns null when cursor is on a non-VariableLookup node', async () => { + const source = '{% assign x = "hello" %}'; + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 0, character: 16 }, // on the string "hello" + }; + + const result = await provider.definitions(params); + assert(result === null); + }); + + it('returns null when variable is used before any assign', async () => { + const source = ['{{ x }}', '{% assign x = 1 %}'].join('\n'); + documentManager.open('file:///test.liquid', source, 1); + + // character 4 = end of 'x' in {{ x }}, lands on VariableLookup + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 0, character: 4 }, + }; + + const result = await provider.definitions(params); + assert(result === null); + }); + + it('works with variables used in filters', async () => { + const source = ['{% assign greeting = "hello" %}', '{{ greeting | upcase }}'].join('\n'); + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 1, character: 5 }, // cursor on "greeting" in {{ greeting | upcase }} + }; + + const result = await provider.definitions(params); + assert(result); + expect(result).toHaveLength(1); + assert(LocationLink.is(result[0])); + expect(result[0].targetRange.start.line).toBe(0); + }); + + it('distinguishes between different variable names', async () => { + const source = ['{% assign foo = 1 %}', '{% assign bar = 2 %}', '{{ foo }}', '{{ bar }}'].join( + '\n', + ); + documentManager.open('file:///test.liquid', source, 1); + + // {{ foo }} should jump to assign foo (line 0) + const paramsFoo: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 2, character: 4 }, + }; + const resultFoo = await provider.definitions(paramsFoo); + assert(resultFoo); + expect(resultFoo).toHaveLength(1); + assert(LocationLink.is(resultFoo[0])); + expect(resultFoo[0].targetRange.start.line).toBe(0); + + // {{ bar }} should jump to assign bar (line 1) + const paramsBar: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 3, character: 4 }, + }; + const resultBar = await provider.definitions(paramsBar); + assert(resultBar); + expect(resultBar).toHaveLength(1); + assert(LocationLink.is(resultBar[0])); + expect(resultBar[0].targetRange.start.line).toBe(1); + }); +}); diff --git a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts new file mode 100644 index 000000000..6721a112e --- /dev/null +++ b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts @@ -0,0 +1,110 @@ +import { LiquidHtmlNode, NodeTypes } from '@shopify/liquid-html-parser'; +import { SourceCodeType, visit } from '@shopify/theme-check-common'; +import { + DefinitionLink, + DefinitionParams, + LocationLink, + Range, +} from 'vscode-languageserver-protocol'; +import { DocumentManager } from '../../documents'; +import { BaseDefinitionProvider } from '../BaseDefinitionProvider'; + +interface AssignDefinition { + /** The name of the variable being assigned */ + name: string; + /** The offset of the start of the name within the assign markup */ + nameStart: number; + /** The offset of the end of the name within the assign markup */ + nameEnd: number; +} + +/** + * Provides go-to-definition for Liquid variable references. + * + * When the cursor is on a `VariableLookup` (e.g. `{{ my_var }}`), this provider + * finds the `{% assign my_var = ... %}` that defines it. + * + * For now, this only handles `assign` tags. Future expansion will add support + * for `capture`, `for`, `tablerow`, and `@param`. + */ +export class VariableDefinitionProvider implements BaseDefinitionProvider { + constructor(private documentManager: DocumentManager) {} + + async definitions( + params: DefinitionParams, + node: LiquidHtmlNode, + _ancestors: LiquidHtmlNode[], + ): Promise { + // Only handle VariableLookup nodes with a non-null name + if (node.type !== NodeTypes.VariableLookup || node.name === null) { + return []; + } + + const variableName = node.name; + const cursorOffset = node.position.start; + + const sourceCode = this.documentManager.get(params.textDocument.uri); + if ( + !sourceCode || + sourceCode.type !== SourceCodeType.LiquidHtml || + sourceCode.ast instanceof Error + ) { + return []; + } + + // Collect all assign definitions for this variable name + const assignDefinitions = visit( + sourceCode.ast, + { + AssignMarkup(assignNode) { + if (assignNode.name !== variableName) return; + return { + name: assignNode.name, + nameStart: assignNode.position.start, + nameEnd: assignNode.position.start + assignNode.name.length, + }; + }, + }, + ); + + if (assignDefinitions.length === 0) { + return []; + } + + // Find the last assign that appears before the cursor position. + // This handles shadowing: later assigns win for later references. + let bestDefinition: AssignDefinition | undefined; + for (const def of assignDefinitions) { + if (def.nameStart < cursorOffset) { + if (!bestDefinition || def.nameStart > bestDefinition.nameStart) { + bestDefinition = def; + } + } + } + + if (!bestDefinition) { + return []; + } + + const { textDocument } = sourceCode; + + const targetRange = Range.create( + textDocument.positionAt(bestDefinition.nameStart), + textDocument.positionAt(bestDefinition.nameEnd), + ); + + const originRange = Range.create( + textDocument.positionAt(node.position.start), + textDocument.positionAt(node.position.end), + ); + + return [ + LocationLink.create( + params.textDocument.uri, + targetRange, + targetRange, + originRange, + ), + ]; + } +} From 2fd7d69df721b581d457b89e743e0ca34334b96b Mon Sep 17 00:00:00 2001 From: James Meng Date: Mon, 16 Mar 2026 16:08:55 -0700 Subject: [PATCH 2/4] Return all preceding assigns for ambiguous definitions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of picking only the last assign before cursor, return all of them. This handles conditional branches (if/else) where multiple assigns could be the definition at runtime: {% if condition %} {% assign x = "from if" %} {% else %} {% assign x = "from else" %} {% endif %} {{ x }} ← peek menu shows both assigns Single assign still jumps directly. Multiple results trigger the editor's peek list — standard LSP pattern for ambiguous definitions, same as TypeScript does for overloaded methods. Co-authored-by: Claude --- .../VariableDefinitionProvider.spec.ts | 39 +++++++++++++++--- .../providers/VariableDefinitionProvider.ts | 40 +++++++++---------- 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts index bf88125f2..c8f2c165f 100644 --- a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts +++ b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts @@ -38,12 +38,11 @@ describe('Module: VariableDefinitionProvider', () => { expect(result[0].targetRange).toEqual(result[0].targetSelectionRange); }); - it('handles variable shadowing — each reference jumps to the correct assign', async () => { + it('returns all preceding assigns — single assign yields direct jump', async () => { const source = ['{% assign x = 1 %}', '{{ x }}', '{% assign x = 2 %}', '{{ x }}'].join('\n'); documentManager.open('file:///test.liquid', source, 1); - // First {{ x }} on line 1 — should jump to first assign (line 0) - // character 4 = end of 'x' (offset lands inside VariableLookup range) + // First {{ x }} on line 1 — only one assign before it → single result (direct jump) const params1: DefinitionParams = { textDocument: { uri: 'file:///test.liquid' }, position: { line: 1, character: 4 }, @@ -54,16 +53,18 @@ describe('Module: VariableDefinitionProvider', () => { assert(LocationLink.is(result1[0])); expect(result1[0].targetRange.start.line).toBe(0); - // Second {{ x }} on line 3 — should jump to second assign (line 2) + // Second {{ x }} on line 3 — two assigns before it → both returned (peek menu) const params2: DefinitionParams = { textDocument: { uri: 'file:///test.liquid' }, position: { line: 3, character: 4 }, }; const result2 = await provider.definitions(params2); assert(result2); - expect(result2).toHaveLength(1); + expect(result2).toHaveLength(2); assert(LocationLink.is(result2[0])); - expect(result2[0].targetRange.start.line).toBe(2); + assert(LocationLink.is(result2[1])); + expect(result2[0].targetRange.start.line).toBe(0); + expect(result2[1].targetRange.start.line).toBe(2); }); it('returns null for global/contextual variables (no assign exists)', async () => { @@ -150,6 +151,32 @@ describe('Module: VariableDefinitionProvider', () => { expect(result[0].targetRange.start.line).toBe(0); }); + it('returns all branch assigns for conditional assignment', async () => { + const source = [ + '{% if condition %}', + ' {% assign x = "from if" %}', + '{% else %}', + ' {% assign x = "from else" %}', + '{% endif %}', + '{{ x }}', + ].join('\n'); + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 5, character: 4 }, + }; + + const result = await provider.definitions(params); + assert(result); + // Both branch assigns are returned — editor shows peek menu + expect(result).toHaveLength(2); + assert(LocationLink.is(result[0])); + assert(LocationLink.is(result[1])); + expect(result[0].targetRange.start.line).toBe(1); + expect(result[1].targetRange.start.line).toBe(3); + }); + it('distinguishes between different variable names', async () => { const source = ['{% assign foo = 1 %}', '{% assign bar = 2 %}', '{{ foo }}', '{{ bar }}'].join( '\n', diff --git a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts index 6721a112e..8f6c5c5f4 100644 --- a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts +++ b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts @@ -24,6 +24,11 @@ interface AssignDefinition { * When the cursor is on a `VariableLookup` (e.g. `{{ my_var }}`), this provider * finds the `{% assign my_var = ... %}` that defines it. * + * Returns **all** assigns before the cursor, not just the last one. This handles + * conditional branches (if/else) where multiple assigns could be the "real" + * definition at runtime. With a single result the editor jumps directly; with + * multiple results the editor shows a peek list. + * * For now, this only handles `assign` tags. Future expansion will add support * for `capture`, `for`, `tablerow`, and `@param`. */ @@ -71,40 +76,35 @@ export class VariableDefinitionProvider implements BaseDefinitionProvider { return []; } - // Find the last assign that appears before the cursor position. - // This handles shadowing: later assigns win for later references. - let bestDefinition: AssignDefinition | undefined; - for (const def of assignDefinitions) { - if (def.nameStart < cursorOffset) { - if (!bestDefinition || def.nameStart > bestDefinition.nameStart) { - bestDefinition = def; - } - } - } + // Return all assigns that appear before the cursor position. + // When there's one (the common case), the editor jumps directly. + // When there are multiple (conditional branches, re-assignment), + // the editor shows a peek list so the user can pick. + const defsBeforeCursor = assignDefinitions.filter((def) => def.nameStart < cursorOffset); - if (!bestDefinition) { + if (defsBeforeCursor.length === 0) { return []; } const { textDocument } = sourceCode; - const targetRange = Range.create( - textDocument.positionAt(bestDefinition.nameStart), - textDocument.positionAt(bestDefinition.nameEnd), - ); - const originRange = Range.create( textDocument.positionAt(node.position.start), textDocument.positionAt(node.position.end), ); - return [ - LocationLink.create( + return defsBeforeCursor.map((def) => { + const targetRange = Range.create( + textDocument.positionAt(def.nameStart), + textDocument.positionAt(def.nameEnd), + ); + + return LocationLink.create( params.textDocument.uri, targetRange, targetRange, originRange, - ), - ]; + ); + }); } } From 5277ec5043e4599b570734def73679c98a9c01c1 Mon Sep 17 00:00:00 2001 From: James Meng Date: Mon, 16 Mar 2026 17:52:53 -0700 Subject: [PATCH 3/4] add failing tests Co-authored-by: AI --- .../VariableDefinitionProvider.spec.ts | 47 +++++++++++++++++++ .../providers/VariableDefinitionProvider.ts | 28 ++++------- 2 files changed, 57 insertions(+), 18 deletions(-) diff --git a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts index c8f2c165f..8ee4d7005 100644 --- a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts +++ b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.spec.ts @@ -205,4 +205,51 @@ describe('Module: VariableDefinitionProvider', () => { assert(LocationLink.is(resultBar[0])); expect(resultBar[0].targetRange.start.line).toBe(1); }); + + it('returns null for variables shadowed by a for loop binding', async () => { + const source = [ + '{% assign x = 1 %}', + '{% for x in (1..2) %}', + ' {{ x }}', + '{% endfor %}', + ].join('\n'); + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 2, character: 6 }, + }; + + const result = await provider.definitions(params); + expect(result).toBeNull(); + }); + + it('does not treat the current assign as a definition for a self-referential RHS lookup', async () => { + const source = ['{% assign x = 1 %}', '{% assign x = x | plus: 1 %}'].join('\n'); + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 1, character: 15 }, + }; + + const result = await provider.definitions(params); + assert(result); + expect(result).toHaveLength(1); + assert(LocationLink.is(result[0])); + expect(result[0].targetRange.start.line).toBe(0); + }); + + it('returns null for a self-referential assign with no prior definition', async () => { + const source = '{% assign x = x | plus: 1 %}'; + documentManager.open('file:///test.liquid', source, 1); + + const params: DefinitionParams = { + textDocument: { uri: 'file:///test.liquid' }, + position: { line: 0, character: 15 }, + }; + + const result = await provider.definitions(params); + expect(result).toBeNull(); + }); }); diff --git a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts index 8f6c5c5f4..78bf24074 100644 --- a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts +++ b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts @@ -58,19 +58,16 @@ export class VariableDefinitionProvider implements BaseDefinitionProvider { } // Collect all assign definitions for this variable name - const assignDefinitions = visit( - sourceCode.ast, - { - AssignMarkup(assignNode) { - if (assignNode.name !== variableName) return; - return { - name: assignNode.name, - nameStart: assignNode.position.start, - nameEnd: assignNode.position.start + assignNode.name.length, - }; - }, + const assignDefinitions = visit(sourceCode.ast, { + AssignMarkup(assignNode) { + if (assignNode.name !== variableName) return; + return { + name: assignNode.name, + nameStart: assignNode.position.start, + nameEnd: assignNode.position.start + assignNode.name.length, + }; }, - ); + }); if (assignDefinitions.length === 0) { return []; @@ -99,12 +96,7 @@ export class VariableDefinitionProvider implements BaseDefinitionProvider { textDocument.positionAt(def.nameEnd), ); - return LocationLink.create( - params.textDocument.uri, - targetRange, - targetRange, - originRange, - ); + return LocationLink.create(params.textDocument.uri, targetRange, targetRange, originRange); }); } } From 0486a5cb41d53d978cbc78b8f29a673ecbc24528 Mon Sep 17 00:00:00 2001 From: James Meng Date: Mon, 16 Mar 2026 18:04:09 -0700 Subject: [PATCH 4/4] handle scope and self-referential variable definitions Co-authored-by: AI --- .../providers/VariableDefinitionProvider.ts | 71 +++++++++++++++---- 1 file changed, 57 insertions(+), 14 deletions(-) diff --git a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts index 78bf24074..25f7e5b1a 100644 --- a/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts +++ b/packages/theme-language-server-common/src/definitions/providers/VariableDefinitionProvider.ts @@ -1,4 +1,4 @@ -import { LiquidHtmlNode, NodeTypes } from '@shopify/liquid-html-parser'; +import { LiquidHtmlNode, NamedTags, NodeTypes } from '@shopify/liquid-html-parser'; import { SourceCodeType, visit } from '@shopify/theme-check-common'; import { DefinitionLink, @@ -10,24 +10,30 @@ import { DocumentManager } from '../../documents'; import { BaseDefinitionProvider } from '../BaseDefinitionProvider'; interface AssignDefinition { - /** The name of the variable being assigned */ - name: string; + /** The offset of the start of the assign markup */ + assignStart: number; + /** The offset of the end of the assign markup */ + assignEnd: number; /** The offset of the start of the name within the assign markup */ nameStart: number; /** The offset of the end of the name within the assign markup */ nameEnd: number; } +interface Scope { + start: number; + end: number; +} + /** * Provides go-to-definition for Liquid variable references. * * When the cursor is on a `VariableLookup` (e.g. `{{ my_var }}`), this provider * finds the `{% assign my_var = ... %}` that defines it. * - * Returns **all** assigns before the cursor, not just the last one. This handles - * conditional branches (if/else) where multiple assigns could be the "real" - * definition at runtime. With a single result the editor jumps directly; with - * multiple results the editor shows a peek list. + * Returns all matching assigns that are visible from the lookup location. + * With a single result the editor jumps directly; with multiple results the + * editor shows a peek list. * * For now, this only handles `assign` tags. Future expansion will add support * for `capture`, `for`, `tablerow`, and `@param`. @@ -38,7 +44,7 @@ export class VariableDefinitionProvider implements BaseDefinitionProvider { async definitions( params: DefinitionParams, node: LiquidHtmlNode, - _ancestors: LiquidHtmlNode[], + ancestors: LiquidHtmlNode[], ): Promise { // Only handle VariableLookup nodes with a non-null name if (node.type !== NodeTypes.VariableLookup || node.name === null) { @@ -47,6 +53,7 @@ export class VariableDefinitionProvider implements BaseDefinitionProvider { const variableName = node.name; const cursorOffset = node.position.start; + const visibleAssignScope = localAssignScope(variableName, node, ancestors); const sourceCode = this.documentManager.get(params.textDocument.uri); if ( @@ -62,7 +69,8 @@ export class VariableDefinitionProvider implements BaseDefinitionProvider { AssignMarkup(assignNode) { if (assignNode.name !== variableName) return; return { - name: assignNode.name, + assignStart: assignNode.position.start, + assignEnd: assignNode.position.end, nameStart: assignNode.position.start, nameEnd: assignNode.position.start + assignNode.name.length, }; @@ -73,11 +81,20 @@ export class VariableDefinitionProvider implements BaseDefinitionProvider { return []; } - // Return all assigns that appear before the cursor position. - // When there's one (the common case), the editor jumps directly. - // When there are multiple (conditional branches, re-assignment), - // the editor shows a peek list so the user can pick. - const defsBeforeCursor = assignDefinitions.filter((def) => def.nameStart < cursorOffset); + // Only return assigns that are visible from the lookup location. + // Using assignEnd avoids treating the current assign as a definition for + // self-referential lookups like `{% assign x = x | plus: 1 %}`. + const defsBeforeCursor = assignDefinitions.filter((def) => { + if (def.assignEnd > cursorOffset) { + return false; + } + + if (!visibleAssignScope) { + return true; + } + + return def.assignStart >= visibleAssignScope.start && def.assignEnd <= visibleAssignScope.end; + }); if (defsBeforeCursor.length === 0) { return []; @@ -100,3 +117,29 @@ export class VariableDefinitionProvider implements BaseDefinitionProvider { }); } } + +function localAssignScope( + variableName: string, + node: LiquidHtmlNode, + ancestors: LiquidHtmlNode[], +): Scope | undefined { + for (let index = ancestors.length - 1; index >= 0; index--) { + const ancestor = ancestors[index]; + + if ( + ancestor.type === NodeTypes.LiquidTag && + (ancestor.name === NamedTags.for || ancestor.name === NamedTags.tablerow) && + typeof ancestor.markup !== 'string' && + ancestor.markup.variableName === variableName && + ancestor.blockStartPosition && + ancestor.blockEndPosition && + node.position.start >= ancestor.blockStartPosition.start && + node.position.start <= ancestor.blockEndPosition.end + ) { + return { + start: ancestor.blockStartPosition.start, + end: ancestor.blockEndPosition.end, + }; + } + } +}