fix(theme-language-server-common): preserve element type on array-returning filters

.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.

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];

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<string> = 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: {