fix: false positives with violation reporting helper function in no-unused-message-ids rule (#290)

Author: bmish

Diff for: 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 for: lib/utils.js

@@ -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 for: 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 for: 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 for: 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
+        )
+      );
+    });
+  });
 });