diff --git a/.changeset/preserve-element-type-on-array-filters.md b/.changeset/preserve-element-type-on-array-filters.md new file mode 100644 index 000000000..5260504b1 --- /dev/null +++ b/.changeset/preserve-element-type-on-array-filters.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme-language-server-common': patch +--- + +Preserve the element type when piping a typed value through `sort`, `sort_natural`, `reverse`, `uniq`, `compact` or `where`. Previously these filters collapsed the type to `untyped[]` because their `array_value` is declared as `untyped` in the filter docs; they are now treated as pass-through in the type system. Fixes #1086. diff --git a/packages/theme-language-server-common/src/TypeSystem.spec.ts b/packages/theme-language-server-common/src/TypeSystem.spec.ts index 10b297f62..8941beec4 100644 --- a/packages/theme-language-server-common/src/TypeSystem.spec.ts +++ b/packages/theme-language-server-common/src/TypeSystem.spec.ts @@ -316,6 +316,84 @@ describe('Module: TypeSystem', () => { }); }); + describe('when using array-returning filters that preserve the element type', () => { + it('should preserve the element type for sort on a typed array', async () => { + const ast = toLiquidHtmlAST(`{% assign x = product.images | sort %}`); + const xVariable = (ast as any).children[0].markup as AssignMarkup; + const inferredType = await typeSystem.inferType(xVariable, ast, 'file:///file.liquid'); + expect(inferredType).to.eql({ kind: 'array', valueType: 'image' }); + }); + + it('should preserve the element type for where on a typed array', async () => { + const ast = toLiquidHtmlAST(`{% assign x = product.images | where: 'featured', true %}`); + const xVariable = (ast as any).children[0].markup as AssignMarkup; + const inferredType = await typeSystem.inferType(xVariable, ast, 'file:///file.liquid'); + expect(inferredType).to.eql({ kind: 'array', valueType: 'image' }); + }); + + ['reverse', 'uniq', 'compact', 'sort_natural'].forEach((name) => { + it(`should preserve the element type for ${name} on a typed array`, async () => { + const ast = toLiquidHtmlAST(`{% assign x = product.images | ${name} %}`); + const xVariable = (ast as any).children[0].markup as AssignMarkup; + const inferredType = await typeSystem.inferType(xVariable, ast, 'file:///file.liquid'); + expect(inferredType).to.eql({ kind: 'array', valueType: 'image' }); + }); + }); + + it('should wrap a scalar input into an array with the scalar type (issue #1086)', async () => { + const ast = toLiquidHtmlAST(` + {% for media in product.images %} + {% assign media_as_array = media | sort %} + {% endfor %} + `); + const forLoop = ast.children[0]; + assert(isNamedLiquidTag(forLoop, NamedTags.for) && forLoop.children?.length === 1); + const branch = forLoop.children[0]; + assert(branch.type === NodeTypes.LiquidBranch); + const assignTag = branch.children.find( + (c: any) => c.type === NodeTypes.LiquidTag && c.name === NamedTags.assign, + ); + assert(assignTag && (assignTag as any).markup); + const inferredType = await typeSystem.inferType( + (assignTag as any).markup as AssignMarkup, + ast, + 'file:///file.liquid', + ); + expect(inferredType).to.eql({ kind: 'array', valueType: 'image' }); + }); + + it('should thread the element type through a chain of pass-through filters', async () => { + const ast = toLiquidHtmlAST(`{% assign x = product.images | sort | reverse %}`); + const xVariable = (ast as any).children[0].markup as AssignMarkup; + const inferredType = await typeSystem.inferType(xVariable, ast, 'file:///file.liquid'); + expect(inferredType).to.eql({ kind: 'array', valueType: 'image' }); + }); + + it('should defer to a non-pass-through filter at the end of the chain', async () => { + const ast = toLiquidHtmlAST(`{% assign x = product.images | sort | size %}`); + const xVariable = (ast as any).children[0].markup as AssignMarkup; + const inferredType = await typeSystem.inferType(xVariable, ast, 'file:///file.liquid'); + expect(inferredType).to.equal('number'); + }); + + it('should fall back to untyped when an unknown filter follows a pass-through', async () => { + const ast = toLiquidHtmlAST(`{% assign x = product.images | sort | not_a_real_filter %}`); + const xVariable = (ast as any).children[0].markup as AssignMarkup; + const inferredType = await typeSystem.inferType(xVariable, ast, 'file:///file.liquid'); + expect(inferredType).to.equal('untyped'); + }); + + it('should still honor `default` as the terminal filter', async () => { + const ast = toLiquidHtmlAST(` + {% assign d = product.featured_image %} + {% assign x = unknown | sort | default: d %} + `); + const xVariable = (ast as any).children[1].markup as AssignMarkup; + const inferredType = await typeSystem.inferType(xVariable, ast, 'file:///file.liquid'); + expect(inferredType).to.equal('image'); + }); + }); + it('should return the type of variables in for loop', async () => { const ast = toLiquidHtmlAST(`{% for item in all_products %}{{ item }}{% endfor %}`); const forLoop = ast.children[0]; diff --git a/packages/theme-language-server-common/src/TypeSystem.ts b/packages/theme-language-server-common/src/TypeSystem.ts index 9b8859d3c..55e83cf40 100644 --- a/packages/theme-language-server-common/src/TypeSystem.ts +++ b/packages/theme-language-server-common/src/TypeSystem.ts @@ -604,6 +604,29 @@ function resolveTypeRangeType( } } +/** + * Filters whose return type is an array of the same element type as their + * input. The filter docs in `filters.json` declare `array_value: "untyped"` + * for these, which would make every pipeline collapse to `untyped[]`. We + * special-case them here to preserve the element type of the input. + * + * Intentionally excluded: + * - `map` — projects a property, needs lookup resolution on the element type. + * - `concat` — merges two arrays, element type would be a union. + */ +const ARRAY_PASSTHROUGH_FILTERS: ReadonlySet = new Set([ + 'sort', + 'sort_natural', + 'reverse', + 'uniq', + 'compact', + 'where', +]); + +function elementTypeOf(t: PseudoType | ArrayType): PseudoType { + return isArrayType(t) ? t.valueType : t; +} + function inferType( thing: Identifier | ComplexLiquidExpression | LiquidVariable | AssignMarkup, symbolsTable: SymbolsTable, @@ -651,20 +674,38 @@ function inferType( // The type is the return value of the last filter // {{ y.property | filter1 | filter2 }} case NodeTypes.LiquidVariable: { - if (thing.filters.length > 0) { - const lastFilter = thing.filters.at(-1)!; - if (lastFilter.name === 'default') { - // default filter is a special case, we need to return the type of the expression - // instead of the filter. - if (lastFilter.args.length > 0 && lastFilter.args[0].type !== NodeTypes.NamedArgument) { - return inferType(lastFilter.args[0], symbolsTable, objectMap, filtersMap); - } - } - const filterEntry = filtersMap[lastFilter.name]; - return filterEntry ? filterEntryReturnType(filterEntry) : Untyped; - } else { + if (thing.filters.length === 0) { return inferType(thing.expression, symbolsTable, objectMap, filtersMap); } + + const lastFilter = thing.filters.at(-1)!; + if (lastFilter.name === 'default') { + // default filter is a special case, we need to return the type of the expression + // instead of the filter. + if (lastFilter.args.length > 0 && lastFilter.args[0].type !== NodeTypes.NamedArgument) { + return inferType(lastFilter.args[0], symbolsTable, objectMap, filtersMap); + } + } + + // Reduce left-to-right through the filter chain so that array-returning + // filters that preserve the element type (e.g. `sort`, `where`, `reverse`) + // can thread the inferred type through the pipeline instead of collapsing + // everything to `untyped[]`. + let current: PseudoType | ArrayType = inferType( + thing.expression, + symbolsTable, + objectMap, + filtersMap, + ); + for (const filter of thing.filters) { + if (ARRAY_PASSTHROUGH_FILTERS.has(filter.name)) { + current = arrayType(elementTypeOf(current)); + continue; + } + const filterEntry = filtersMap[filter.name]; + current = filterEntry ? filterEntryReturnType(filterEntry) : Untyped; + } + return current; } default: {