Merge pull request #2677 from johanrd/night_fix/template-no-action

Post-merge-review: Fix `template-no-action` false positive in GJS/GTS

Author: NullVoxPopuli

docs/rules/template-no-action.md

@@ -42,6 +42,25 @@ Examples of **correct** code for this rule:
 </template>
 ```
 
+```gjs
+import action from './my-action-helper';
+<template>
+  {{action this.handleClick}}
+</template>
+```
+
+```gjs
+<template>
+  {{#each items as |action|}}
+    <button {{action this.handleClick}}>x</button>
+  {{/each}}
+</template>
+```
+
+## Strict-mode behavior
+
+`action` is an ambient strict-mode keyword in Ember (registered in `STRICT_MODE_KEYWORDS`), so `{{action this.x}}` works in `.gjs`/`.gts` templates without an explicit import. The rule still flags those uses to discourage the deprecated keyword — but skips reports when `action` resolves to a JS-scope binding (an import or local declaration) or a template block param.
+
 ## Migration
 
 - Replace `(action "methodName")` with method references or `(fn this.methodName)`

lib/rules/template-no-action.js

@@ -1,20 +1,19 @@
-function isActionHelper(node) {
+function isActionHelperPath(node) {
   if (!node.path || node.path.type !== 'GlimmerPathExpression') {
     return false;
   }
 
   // Check if it's the action helper (not this.action or @action)
   const path = node.path;
-  if (path.original === 'action') {
-    // Check head.type to avoid deprecated data/this properties
-    const head = path.head;
-    if (head && (head.type === 'AtHead' || head.type === 'ThisHead')) {
-      return false;
-    }
-    return true;
+  if (path.original !== 'action') {
+    return false;
   }
-
-  return false;
+  // Avoid deprecated data/this properties — those are not the helper.
+  const head = path.head;
+  if (head && (head.type === 'AtHead' || head.type === 'ThisHead')) {
+    return false;
+  }
+  return true;
 }
 
 /** @type {import('eslint').Rule.RuleModule} */
@@ -31,39 +30,61 @@ module.exports = {
     schema: [],
     messages: {
       subExpression:
-        'Do not use `action` as (action ...). Instead, use the `on` modifier and `fn` helper.',
-      mustache: 'Do not use `action` in templates. Instead, use the `on` modifier and `fn` helper.',
+        'Do not use `action` as (action ...) — deprecated in Ember 5.9, removed in 6.0. Use the `fn` helper instead.',
+      mustache:
+        'Do not use `action` in templates — deprecated in Ember 5.9, removed in 6.0. Use the `on` modifier and `fn` helper instead.',
       modifier:
-        'Do not use `action` as an element modifier. Instead, use the `on` modifier and `fn` helper.',
+        'Do not use `action` as an element modifier — deprecated in Ember 5.9, removed in 6.0. Use the `on` modifier instead.',
     },
   },
 
   create(context) {
+    // `action` is an ambient strict-mode keyword in Ember (registered in
+    // STRICT_MODE_KEYWORDS), so `{{action this.x}}` works in .gjs/.gts without
+    // an import. Still flag the ambient keyword everywhere — but skip when
+    // `action` resolves to a binding (JS import/const, or template block param).
+    // ember-eslint-parser already registers template block params in scope, so
+    // a single getScope walk covers both.
+    const sourceCode = context.sourceCode;
+
+    function isInScope(node) {
+      if (!sourceCode) {
+        return false;
+      }
+      try {
+        let scope = sourceCode.getScope(node);
+        while (scope) {
+          if (scope.variables.some((v) => v.name === 'action')) {
+            return true;
+          }
+          scope = scope.upper;
+        }
+      } catch {
+        // getScope not available in .hbs-only mode
+      }
+      return false;
+    }
+
+    function shouldFlag(node) {
+      return isActionHelperPath(node) && !isInScope(node);
+    }
+
     return {
       GlimmerSubExpression(node) {
-        if (isActionHelper(node)) {
-          context.report({
-            node,
-            messageId: 'subExpression',
-          });
+        if (shouldFlag(node)) {
+          context.report({ node, messageId: 'subExpression' });
         }
       },
 
       GlimmerMustacheStatement(node) {
-        if (isActionHelper(node)) {
-          context.report({
-            node,
-            messageId: 'mustache',
-          });
+        if (shouldFlag(node)) {
+          context.report({ node, messageId: 'mustache' });
         }
       },
 
       GlimmerElementModifierStatement(node) {
-        if (isActionHelper(node)) {
-          context.report({
-            node,
-            messageId: 'modifier',
-          });
+        if (shouldFlag(node)) {
+          context.report({ node, messageId: 'modifier' });
         }
       },
     };

tests/lib/rules/template-no-action.js

@@ -28,6 +28,37 @@ ruleTester.run('template-no-action', rule, {
     `<template>
       {{@action}}
     </template>`,
+
+    // GJS/GTS: a JS-scope binding shadows the ambient `action` keyword. The
+    // user has imported (or declared) their own `action`, so flagging would
+    // corrupt their reference.
+    {
+      filename: 'test.gjs',
+      code: "import action from './my-action';\n<template>{{action this.handleClick}}</template>",
+    },
+    {
+      filename: 'test.gjs',
+      code: "import action from './my-action';\n<template>{{(action this.handleClick)}}</template>",
+    },
+    {
+      filename: 'test.gjs',
+      code: "import action from './my-action';\n<template><button {{action this.handleClick}}>x</button></template>",
+    },
+    {
+      filename: 'test.gts',
+      code: "import action from './my-action';\n<template>{{action this.handleClick}}</template>",
+    },
+    {
+      filename: 'test.gjs',
+      code: 'const action = (h) => () => h();\n<template>{{action this.handleClick}}</template>',
+    },
+
+    // Template block-param shadowing — `action` is the iterator/let-bound
+    // value, not the ambient keyword.
+    '<template>{{#each items as |action|}}{{action this.x}}{{/each}}</template>',
+    '<template>{{#let (component "x") as |action|}}{{action this.x}}{{/let}}</template>',
+    '<template>{{#each items as |action|}}<button {{action this.x}}>x</button>{{/each}}</template>',
+    '<template><Foo as |action|>{{action this.x}}</Foo></template>',
   ],
 
   invalid: [
@@ -39,7 +70,7 @@ ruleTester.run('template-no-action', rule, {
       errors: [
         {
           message:
-            'Do not use `action` as (action ...). Instead, use the `on` modifier and `fn` helper.',
+            'Do not use `action` as (action ...) — deprecated in Ember 5.9, removed in 6.0. Use the `fn` helper instead.',
           type: 'GlimmerSubExpression',
         },
       ],
@@ -52,7 +83,7 @@ ruleTester.run('template-no-action', rule, {
       errors: [
         {
           message:
-            'Do not use `action` in templates. Instead, use the `on` modifier and `fn` helper.',
+            'Do not use `action` in templates — deprecated in Ember 5.9, removed in 6.0. Use the `on` modifier and `fn` helper instead.',
           type: 'GlimmerMustacheStatement',
         },
       ],
@@ -65,7 +96,7 @@ ruleTester.run('template-no-action', rule, {
       errors: [
         {
           message:
-            'Do not use `action` as an element modifier. Instead, use the `on` modifier and `fn` helper.',
+            'Do not use `action` as an element modifier — deprecated in Ember 5.9, removed in 6.0. Use the `on` modifier instead.',
           type: 'GlimmerElementModifierStatement',
         },
       ],
@@ -78,10 +109,49 @@ ruleTester.run('template-no-action', rule, {
       errors: [
         {
           message:
-            'Do not use `action` in templates. Instead, use the `on` modifier and `fn` helper.',
+            'Do not use `action` in templates — deprecated in Ember 5.9, removed in 6.0. Use the `on` modifier and `fn` helper instead.',
           type: 'GlimmerMustacheStatement',
         },
       ],
     },
+
+    // GJS/GTS: ambient `action` keyword usage with no shadowing import or
+    // block param. Still flagged.
+    {
+      filename: 'test.gjs',
+      code: '<template>{{action "save"}}</template>',
+      output: null,
+      errors: [{ messageId: 'mustache', type: 'GlimmerMustacheStatement' }],
+    },
+    {
+      filename: 'test.gjs',
+      code: '<template><button {{on "click" (action "save")}}>x</button></template>',
+      output: null,
+      errors: [{ messageId: 'subExpression', type: 'GlimmerSubExpression' }],
+    },
+    {
+      filename: 'test.gts',
+      code: '<template><button {{action "submit"}}>x</button></template>',
+      output: null,
+      errors: [{ messageId: 'modifier', type: 'GlimmerElementModifierStatement' }],
+    },
+
+    // Unrelated JS bindings do NOT mask the rule. Only a binding named
+    // `action` should be respected.
+    {
+      filename: 'test.gjs',
+      code: "import handler from './handler';\n<template>{{action this.x}}</template>",
+      output: null,
+      errors: [{ messageId: 'mustache' }],
+    },
+
+    // Ambient `action` outside a block-param scope is still flagged, even
+    // when an inner block legitimately shadows it.
+    {
+      filename: 'test.gjs',
+      code: '<template>{{#each items as |action|}}{{action this.x}}{{/each}}{{action this.y}}</template>',
+      output: null,
+      errors: [{ messageId: 'mustache' }],
+    },
   ],
 });