Update contentFor tag grammer to properly extract arguments

.changeset/short-jobs-boil.md

@@ -0,0 +1,7 @@
+---
+'@shopify/liquid-html-parser': minor
+'@shopify/theme-check-common': minor
+---
+
+Update Ohm grammar for ContentFor tag to extract arguments correctly
+Update ValidContentForArguments check to report deprecated context. argument usage

packages/liquid-html-parser/grammar/liquid-html.ohm

@@ -127,8 +127,13 @@ Liquid <: Helpers {
   liquidTagLiquidMarkup = tagMarkup
 
   liquidTagContentFor = liquidTagRule<"content_for", liquidTagContentForMarkup>
+
   liquidTagContentForMarkup =
-    contentForType (argumentSeparatorOptionalComma tagArguments) (space* ",")? space*
+    contentForType (argumentSeparatorOptionalComma contentForTagArgument) (space* ",")? space*
+
+  contentForTagArgument = listOf<contentForNamedArgument<delimTag>, argumentSeparatorOptionalComma>
+  contentForNamedArgument<delim> = (variableSegment ("." variableSegment)*) space* ":" space* (liquidExpression<delim>)
+
   contentForType = liquidString<delimTag>
 
   liquidTagInclude = liquidTagRule<"include", liquidTagRenderMarkup>

packages/liquid-html-parser/src/stage-1-cst.spec.ts

@@ -604,15 +604,19 @@ describe('Unit: Stage 1 (CST)', () => {
 
       it('should parse content_for "block", id: "my-id", type: "my-block"', () => {
         for (const { toCST, expectPath } of testCases) {
-          cst = toCST(`{% content_for "block", id: "block-id", type: "block-type" -%}`);
+          cst = toCST(
+            `{% content_for "block", closest.product: product, closest.metaobject.test: product, id: "block-id", type: "block-type" -%}`,
+          );
           expectPath(cst, '0.type').to.equal('LiquidTag');
           expectPath(cst, '0.name').to.equal('content_for');
           expectPath(cst, '0.markup.type').to.equal('ContentForMarkup');
           expectPath(cst, '0.markup.contentForType.type').to.equal('String');
           expectPath(cst, '0.markup.contentForType.value').to.equal('block');
           expectPath(cst, '0.markup.contentForType.single').to.equal(false);
-          expectPath(cst, '0.markup.args').to.have.lengthOf(2);
+          expectPath(cst, '0.markup.args').to.have.lengthOf(4);
           const namedArguments = [
+            { name: 'closest.product', valueType: 'VariableLookup' },
+            { name: 'closest.metaobject.test', valueType: 'VariableLookup' },
             { name: 'id', valueType: 'String' },
             { name: 'type', valueType: 'String' },
           ];

packages/liquid-html-parser/src/stage-1-cst.ts

@@ -81,6 +81,7 @@ export enum ConcreteNodeTypes {
   RenderMarkup = 'RenderMarkup',
   PaginateMarkup = 'PaginateMarkup',
   RenderVariableExpression = 'RenderVariableExpression',
+  ContentForNamedArgument = 'ContentForNamedArgument',
 }
 
 export const LiquidLiteralValues = {
@@ -873,6 +874,7 @@ function toCST<T>(
     },
     simpleArgument: 0,
     tagArguments: 0,
+    contentForTagArgument: 0,
     positionalArgument: 0,
     namedArgument: {
       type: ConcreteNodeTypes.NamedArgument,
@@ -883,6 +885,15 @@ function toCST<T>(
       source,
     },
 
+    contentForNamedArgument: {
+      type: ConcreteNodeTypes.NamedArgument,
+      name: (node) => node[0].sourceString + node[1].sourceString,
+      value: 6,
+      locStart,
+      locEnd,
+      source,
+    },
+
     liquidString: 0,
     liquidDoubleQuotedString: {
       type: ConcreteNodeTypes.String,

packages/theme-check-common/src/checks/valid-content-for-arguments/index.spec.ts

@@ -7,29 +7,44 @@ describe('Module: ValidContentForArguments', () => {
     it('should not report offenses for strategies we do not know or support yet', async () => {
       const offenses = await runLiquidCheck(
         ValidContentForArguments,
-        '{% content_for "snippet", context.product: product %}',
+        '{% content_for "snippet", closest.product: product %}',
       );
       expect(offenses).toHaveLength(0);
     });
   });
 
   describe('{% content_for "blocks" %}', () => {
-    it('should accept `context.*` kwargs', async () => {
+    it('should accept `closest.*` kwargs', async () => {
       const offenses = await runLiquidCheck(
         ValidContentForArguments,
-        '{% content_for "blocks", context.product: product %}',
+        '{% content_for "blocks", closest.product: product %}',
       );
       expect(offenses).toHaveLength(0);
     });
 
-    it('should report offenses for non-`context.*` kwargs', async () => {
+    it('should report offenses for non-`closest.*` kwargs', async () => {
       const offenses = await runLiquidCheck(
         ValidContentForArguments,
         '{% content_for "blocks", product: product %}',
       );
       expect(offenses).toHaveLength(1);
       expect(offenses[0]!.message).to.equal(
-        `{% content_for "blocks" %} only accepts 'context.*' arguments`,
+        `{% content_for "blocks" %} only accepts 'closest.*' arguments`,
+      );
+    });
+
+    it('should report offenses for deprecated `context.*` kwargs', async () => {
+      const offenses = await runLiquidCheck(
+        ValidContentForArguments,
+        '{% content_for "blocks", context.product: product %}',
+      );
+      expect(offenses).toHaveLength(2);
+      expect(offenses[0]!.message).to.equal(
+        `{% content_for "blocks" %} only accepts 'closest.*' arguments`,
+      );
+
+      expect(offenses[1]!.message).to.equal(
+        `{% content_for "blocks" %} only accepts 'closest.*' arguments. The 'context.*' arguments usage has been deprecated.`,
       );
     });
   });
@@ -61,14 +76,29 @@ describe('Module: ValidContentForArguments', () => {
       expect(offenses[0].message).to.equal(`The 'id' argument should be a string`);
     });
 
-    it(`should report an offense if other kwargs are passed that are not named 'context.*'`, async () => {
+    it(`should report an offense if other kwargs are passed that are not named 'closest.*'`, async () => {
       const offenses = await runLiquidCheck(
         ValidContentForArguments,
         '{% content_for "block", type: "foo", id: "id", product: product %}',
       );
       expect(offenses).toHaveLength(1);
       expect(offenses[0].message).to.equal(
-        `{% content_for "block" %} only accepts 'id', 'type' and 'context.*' arguments`,
+        `{% content_for "block" %} only accepts 'id', 'type' and 'closest.*' arguments`,
+      );
+    });
+
+    it('should report offenses for deprecated `context.*` kwargs', async () => {
+      const offenses = await runLiquidCheck(
+        ValidContentForArguments,
+        '{% content_for "block", type: "type", id: "static-block", context.product: product %}',
+      );
+      expect(offenses).toHaveLength(2);
+      expect(offenses[0]!.message).to.equal(
+        `{% content_for "block" %} only accepts 'id', 'type' and 'closest.*' arguments`,
+      );
+
+      expect(offenses[1]!.message).to.equal(
+        `{% content_for "block" %} accepts 'closest.*' arguments. The 'context.*' arguments usage has been deprecated.`,
       );
     });
   });

packages/theme-check-common/src/checks/valid-content-for-arguments/index.ts

@@ -1,7 +1,8 @@
 import { ContentForMarkup, NodeTypes } from '@shopify/liquid-html-parser';
 import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';
 
-// content_for "block" and content_for "blocks" only allow `context.*` kwargs.
+// content_for "block" and content_for "blocks" only allow `closest.*` kwargs.
+const isClosestArgument = (argName: string) => argName.startsWith('closest.');
 const isContextArgument = (argName: string) => argName.startsWith('context.');
 
 export const ValidContentForArguments: LiquidCheckDefinition = {
@@ -23,10 +24,19 @@ export const ValidContentForArguments: LiquidCheckDefinition = {
   create(context) {
     const validationStrategies = {
       blocks: (node: ContentForMarkup) => {
-        const problematicArguments = node.args.filter((arg) => !isContextArgument(arg.name));
+        const problematicArguments = node.args.filter((arg) => !isClosestArgument(arg.name));
         for (const arg of problematicArguments) {
           context.report({
-            message: `{% content_for "blocks" %} only accepts 'context.*' arguments`,
+            message: `{% content_for "blocks" %} only accepts 'closest.*' arguments`,
+            startIndex: arg.position.start,
+            endIndex: arg.position.end,
+          });
+        }
+
+        const deprecatedArguments = node.args.filter((arg) => isContextArgument(arg.name));
+        for (const arg of deprecatedArguments) {
+          context.report({
+            message: `{% content_for "blocks" %} only accepts 'closest.*' arguments. The 'context.*' arguments usage has been deprecated.`,
             startIndex: arg.position.start,
             endIndex: arg.position.end,
           });
@@ -62,12 +72,21 @@ export const ValidContentForArguments: LiquidCheckDefinition = {
         }
 
         const problematicArguments = node.args.filter(
-          (arg) => !(requiredArguments.includes(arg.name) || isContextArgument(arg.name)),
+          (arg) => !(requiredArguments.includes(arg.name) || isClosestArgument(arg.name)),
         );
 
         for (const arg of problematicArguments) {
           context.report({
-            message: `{% content_for "block" %} only accepts 'id', 'type' and 'context.*' arguments`,
+            message: `{% content_for "block" %} only accepts 'id', 'type' and 'closest.*' arguments`,
+            startIndex: arg.position.start,
+            endIndex: arg.position.end,
+          });
+        }
+
+        const deprecatedArguments = node.args.filter((arg) => isContextArgument(arg.name));
+        for (const arg of deprecatedArguments) {
+          context.report({
+            message: `{% content_for "block" %} accepts 'closest.*' arguments. The 'context.*' arguments usage has been deprecated.`,
             startIndex: arg.position.start,
             endIndex: arg.position.end,
           });