Repository: not-an-aardvark/eslint-plugin-eslint-plugin (js)

Issue:
False positive with `no-unused-message-ids` when helper function parameter reassignment present
In this example, the `messageId` being reported can come from a function parameter or from parameter reassignment. We should ignore this scenario because we don't know all the possible values the function could be called with.

Example:

```js

checkCall(node, node, 'unsafeCall');

function checkCall(
  reportingNode: TSESTree.Node,
  messageId: MessageIds,
): void {
  if (foo) {
    messageId = 'unsafeCallThis';
  }
  context.report({
    node: reportingNode,
    messageId: messageId,
  });
}
```

https://github.com/typescript-eslint/typescript-eslint/blob/b6e5413fb12c2b1943a288b514f864c339380388/packages/eslint-plugin/src/rules/no-unsafe-call.ts#L63

This is not specific to TypeScript.

Pull request description:
Fix false positives with violation reporting helper function in `no-unused-message-ids` rule
When a `messageId` from a function parameter is reported, stop trying to detect unused `messageId`s, since we can't currently know what values that function  is called with. In a future improvement, we could try to detect the function calls to find the possible values the parameter is called with.

Fixes #282.

Patch:
diff --git a/.eslintrc.js b/.eslintrc.js
index 0c14d96..223fd7f 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -8,6 +8,7 @@ module.exports = {
   },
   extends: [
     'not-an-aardvark/node',
+    'plugin:eslint-comments/recommended',
     'plugin:node/recommended',
     'plugin:prettier/recommended',
     'plugin:unicorn/recommended',
@@ -23,6 +24,9 @@ module.exports = {
     ],
     'require-jsdoc': 'error',
 
+    'eslint-comments/no-unused-disable': 'error',
+    'eslint-comments/require-description': 'error',
+
     'unicorn/consistent-function-scoping': 'off',
     'unicorn/no-array-callback-reference': 'off',
     'unicorn/no-array-for-each': 'off',
@@ -65,6 +69,8 @@ module.exports = {
         'no-unused-vars': 'off',
         strict: 'off',
 
+        'eslint-comments/require-description': 'off',
+
         'unicorn/filename-case': 'off',
       },
     },
diff --git a/lib/index.js b/lib/index.js
index a6892c2..e0a81ce 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -43,7 +43,7 @@ const allRules = Object.fromEntries(
 
 module.exports.rules = allRules;
 
-// eslint-disable-next-line unicorn/prefer-object-from-entries
+// eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now
 module.exports.configs = Object.keys(configFilters).reduce(
   (configs, configName) => {
     return Object.assign(configs, {
diff --git a/lib/rules/no-unused-message-ids.js b/lib/rules/no-unused-message-ids.js
index 8aa4bfb..01fc262 100644
--- a/lib/rules/no-unused-message-ids.js
+++ b/lib/rules/no-unused-message-ids.js
@@ -120,7 +120,8 @@ module.exports = {
 
           if (
             values.length === 0 ||
-            values.some((val) => val.type !== 'Literal')
+            values.some((val) => val.type !== 'Literal') ||
+            utils.isVariableFromParameter(node.value, scopeManager)
           ) {
             // When a dynamic messageId is used and we can't detect its value, disable the rule to avoid false positives.
             hasSeenUnknownMessageId = true;
diff --git a/lib/rules/require-meta-docs-url.js b/lib/rules/require-meta-docs-url.js
index 4932467..a195503 100644
--- a/lib/rules/require-meta-docs-url.js
+++ b/lib/rules/require-meta-docs-url.js
@@ -114,7 +114,7 @@ module.exports = {
 
           messageId: !urlPropNode
             ? 'missing'
-            : // eslint-disable-next-line unicorn/no-nested-ternary
+            : // eslint-disable-next-line unicorn/no-nested-ternary -- this is fine for now
             !expectedUrl
             ? 'wrongType'
             : /* otherwise */ 'mismatch',
diff --git a/lib/utils.js b/lib/utils.js
index d2bc650..3129903 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -41,7 +41,7 @@ const INTERESTING_RULE_KEYS = new Set(['create', 'meta']);
  * @returns Object
  */
 function collectInterestingProperties(properties, interestingKeys) {
-  // eslint-disable-next-line unicorn/prefer-object-from-entries
+  // eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now
   return properties.reduce((parsedProps, prop) => {
     const keyValue = module.exports.getKeyName(prop);
     if (interestingKeys.has(keyValue)) {
@@ -128,7 +128,7 @@ function getRuleExportsESM(ast, scopeManager) {
         ].includes(statement.type)
       )
       .map((statement) => statement.declaration || statement.expression)
-      // eslint-disable-next-line unicorn/prefer-object-from-entries
+      // eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now
       .reduce((currentExports, node) => {
         if (node.type === 'ObjectExpression') {
           // Check `export default { create() {}, meta: {} }`
@@ -184,7 +184,7 @@ function getRuleExportsCJS(ast, scopeManager) {
       .map((statement) => statement.expression)
       .filter((expression) => expression.type === 'AssignmentExpression')
       .filter((expression) => expression.left.type === 'MemberExpression')
-      // eslint-disable-next-line unicorn/prefer-object-from-entries
+      // eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now
       .reduce((currentExports, node) => {
         if (
           node.left.object.type === 'Identifier' &&
@@ -600,7 +600,7 @@ module.exports = {
 
     if (reportArgs.length === 1) {
       if (reportArgs[0].type === 'ObjectExpression') {
-        // eslint-disable-next-line unicorn/prefer-object-from-entries
+        // eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now
         return reportArgs[0].properties.reduce((reportInfo, property) => {
           const propName = module.exports.getKeyName(property);
 
@@ -888,4 +888,19 @@ module.exports = {
       return [];
     });
   },
+
+  /**
+   * Check whether a variable's definition is from a function parameter.
+   * @param {Node} node - the Identifier node for the variable.
+   * @param {ScopeManager} scopeManager
+   * @returns {boolean} whether the variable comes from a function parameter
+   */
+  isVariableFromParameter(node, scopeManager) {
+    const variable = findVariable(
+      scopeManager.acquire(node) || scopeManager.globalScope,
+      node
+    );
+
+    return variable?.defs[0]?.type === 'Parameter';
+  },
 };
diff --git a/package.json b/package.json
index cf38d74..c68fb74 100644
--- a/package.json
+++ b/package.json
@@ -54,6 +54,7 @@
     "eslint": "^8.7.0",
     "eslint-config-not-an-aardvark": "^2.1.0",
     "eslint-config-prettier": "^8.3.0",
+    "eslint-plugin-eslint-comments": "^3.2.0",
     "eslint-plugin-eslint-plugin": "file:./",
     "eslint-plugin-markdown": "^2.0.1",
     "eslint-plugin-node": "^11.1.0",


Test patch:
diff --git a/tests/lib/rules/no-missing-message-ids.js b/tests/lib/rules/no-missing-message-ids.js
index 22a465d..02cd8a0 100644
--- a/tests/lib/rules/no-missing-message-ids.js
+++ b/tests/lib/rules/no-missing-message-ids.js
@@ -196,6 +196,48 @@ ruleTester.run('no-missing-message-ids', rule, {
         }
       };
     `,
+    // Helper function with messageId parameter, outside rule.
+    `
+      function report(node, messageId) {
+        context.report({node, messageId});
+      }
+      module.exports = {
+        meta: { messages: { foo: 'hello' } },
+        create(context) {
+          report(node, 'foo');
+        }
+      };
+    `,
+    // Helper function with messageId parameter, inside rule, with parameter reassignment.
+    `
+      module.exports = {
+        meta: { messages: { foo: 'hello', bar: 'world' } },
+        create(context) {
+          function report(node, messageId) {
+            if (foo) {
+              messageId = 'bar';
+            }
+            context.report({node, messageId});
+          }
+          report(node, 'foo');
+        }
+      };
+    `,
+    // Helper function with messageId parameter, inside rule, with missing messageId.
+    // TODO: this should be an invalid test case because a non-existent `messageId` is used.
+    // Eventually, we should be able to detect what values are passed to this function for its `messageId` parameter.
+    `
+      module.exports = {
+        meta: { messages: { foo: 'hello' } },
+        create(context) {
+          function report(node, messageId) {
+            context.report({node, messageId});
+          }
+          report(node, 'foo');
+          report(node, 'bar');
+        }
+      };
+    `,
   ],
 
   invalid: [
@@ -287,5 +329,29 @@ ruleTester.run('no-missing-message-ids', rule, {
         },
       ],
     },
+    {
+      // Helper function with messageId parameter, inside rule, with missing messageId due to parameter reassignment.
+      code: `
+        module.exports = {
+          meta: { messages: { foo: 'hello' } },
+          create(context) {
+            function report(node, messageId) {
+              if (foo) {
+                messageId = 'bar';
+              }
+              context.report({node, messageId});
+            }
+            report(node, 'foo');
+          }
+        };
+      `,
+      errors: [
+        {
+          messageId: 'missingMessage',
+          data: { messageId: 'bar' },
+          type: 'Literal',
+        },
+      ],
+    },
   ],
 });
diff --git a/tests/lib/rules/no-unused-message-ids.js b/tests/lib/rules/no-unused-message-ids.js
index d35cadf..52e3f80 100644
--- a/tests/lib/rules/no-unused-message-ids.js
+++ b/tests/lib/rules/no-unused-message-ids.js
@@ -227,6 +227,50 @@ ruleTester.run('no-unused-message-ids', rule, {
         create(context) {}
       };
     `,
+    // Helper function messageId parameter, outside rule.
+    `
+      function reportFoo(node, messageId) {
+        context.report({ node, messageId });
+      }
+      module.exports = {
+        meta: { messages: { foo: 'hello', bar: 'world', baz: 'planet' } },
+        create(context) {
+          reportFoo(node, 'foo');
+          reportFoo(node, 'bar');
+          reportFoo(node, 'baz');
+        }
+      };
+    `,
+    // Helper function with messageId parameter, inside rule, parameter reassignment.
+    `
+      module.exports = {
+        meta: { messages: { foo: 'hello', bar: 'world', baz: 'planet' } },
+        create(context) {
+          function reportFoo(node, messageId) {
+            if (foo) {
+              messageId = 'baz';
+            }
+            context.report({ node, messageId });
+          }
+          reportFoo(node, 'foo');
+          reportFoo(node, 'bar');
+        }
+      };
+    `,
+    // Helper function with messageId parameter, outside rule, with an unused messageId.
+    // TODO: this should be an invalid test case because a messageId is unused.
+    // Eventually, we should be able to detect what values are passed to this function for its messageId parameter.
+    `
+      function reportFoo(node, messageId) {
+        context.report({ node, messageId });
+      }
+      module.exports = {
+        meta: { messages: { foo: 'hello', bar: 'world' } },
+        create(context) {
+          reportFoo(node, 'foo');
+        }
+      };
+    `,
   ],
 
   invalid: [
@@ -363,7 +407,7 @@ ruleTester.run('no-unused-message-ids', rule, {
           context.report({ node, messageId });
         }
         module.exports = {
-          meta: { messages: { foo: 'hello world' } },
+          meta: { messages: { foo: 'hello world', bar: 'baz' } },
           create(context) {
             reportFoo(node);
           }
diff --git a/tests/lib/utils.js b/tests/lib/utils.js
index 66e1b02..ebebacc 100644
--- a/tests/lib/utils.js
+++ b/tests/lib/utils.js
@@ -1646,4 +1646,39 @@ describe('utils', () => {
       );
     });
   });
+
+  describe('isVariableFromParameter', function () {
+    it('returns true for function parameter', () => {
+      const code =
+        'function myFunc(x) { if (foo) { x = "abc"; } console.log(x) }; myFunc("def");';
+      const ast = espree.parse(code, {
+        ecmaVersion: 9,
+        range: true,
+      });
+
+      const scopeManager = eslintScope.analyze(ast);
+      assert.ok(
+        utils.isVariableFromParameter(
+          ast.body[0].body.body[1].expression.arguments[0],
+          scopeManager
+        )
+      );
+    });
+
+    it('returns false for const variable', () => {
+      const code = 'const x = "abc"; console.log(x);';
+      const ast = espree.parse(code, {
+        ecmaVersion: 9,
+        range: true,
+      });
+
+      const scopeManager = eslintScope.analyze(ast);
+      assert.notOk(
+        utils.isVariableFromParameter(
+          ast.body[1].expression.arguments[0],
+          scopeManager
+        )
+      );
+    });
+  });
 });