From 01d0eef2885b1626642d925735c9cb59c1e611b8 Mon Sep 17 00:00:00 2001
From: Bryan Mishkin <698306+bmish@users.noreply.github.com>
Date: Fri, 12 Aug 2022 02:32:03 -0400
Subject: [PATCH] fix: false positive with no-unused-message-ids from external
 violation reporting function (#286)

---
 lib/rules/no-unused-message-ids.js       | 52 +++++++++++++++---------
 tests/lib/rules/no-unused-message-ids.js | 27 ++++--------
 2 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/lib/rules/no-unused-message-ids.js b/lib/rules/no-unused-message-ids.js
index df793605..8aa4bfb0 100644
--- a/lib/rules/no-unused-message-ids.js
+++ b/lib/rules/no-unused-message-ids.js
@@ -30,7 +30,8 @@ module.exports = {
 
     const messageIdsUsed = new Set();
     let contextIdentifiers;
-    let shouldPerformUnusedCheck = true;
+    let hasSeenUnknownMessageId = false;
+    let hasSeenViolationReport = false;
 
     const messageIdNodes = utils.getMessageIdNodes(info, scopeManager);
     if (!messageIdNodes) {
@@ -44,22 +45,29 @@ module.exports = {
       },
 
       'Program:exit'() {
-        if (shouldPerformUnusedCheck) {
-          const messageIdNodesUnused = messageIdNodes.filter(
-            (node) =>
-              !messageIdsUsed.has(utils.getKeyName(node, context.getScope()))
-          );
-
-          // Report any messageIds that were never used.
-          for (const messageIdNode of messageIdNodesUnused) {
-            context.report({
-              node: messageIdNode,
-              messageId: 'unusedMessage',
-              data: {
-                messageId: utils.getKeyName(messageIdNode, context.getScope()),
-              },
-            });
-          }
+        if (hasSeenUnknownMessageId || !hasSeenViolationReport) {
+          /*
+          Bail out when the rule is likely to have false positives.
+          - If we saw a dynamic/unknown messageId
+          - If we couldn't find any violation reporting code, likely because a helper function from an external file is handling this
+          */
+          return;
+        }
+
+        const messageIdNodesUnused = messageIdNodes.filter(
+          (node) =>
+            !messageIdsUsed.has(utils.getKeyName(node, context.getScope()))
+        );
+
+        // Report any messageIds that were never used.
+        for (const messageIdNode of messageIdNodesUnused) {
+          context.report({
+            node: messageIdNode,
+            messageId: 'unusedMessage',
+            data: {
+              messageId: utils.getKeyName(messageIdNode, context.getScope()),
+            },
+          });
         }
       },
 
@@ -76,6 +84,8 @@ module.exports = {
             return;
           }
 
+          hasSeenViolationReport = true;
+
           const reportMessagesAndDataArray =
             utils.collectReportViolationAndSuggestionData(reportInfo);
           for (const { messageId } of reportMessagesAndDataArray.filter(
@@ -90,7 +100,7 @@ module.exports = {
               values.some((val) => val.type !== 'Literal')
             ) {
               // When a dynamic messageId is used and we can't detect its value, disable the rule to avoid false positives.
-              shouldPerformUnusedCheck = false;
+              hasSeenUnknownMessageId = true;
             }
             values.forEach((val) => messageIdsUsed.add(val.value));
           }
@@ -101,17 +111,21 @@ module.exports = {
         // In order to reduce false positives, we will also check for messageId properties anywhere in the file.
         // This is helpful especially in the event that helper functions are used for reporting violations.
         if (node.key.type === 'Identifier' && node.key.name === 'messageId') {
+          hasSeenViolationReport = true;
+
           const values =
             node.value.type === 'Literal'
               ? [node.value]
               : utils.findPossibleVariableValues(node.value, scopeManager);
+
           if (
             values.length === 0 ||
             values.some((val) => val.type !== 'Literal')
           ) {
             // When a dynamic messageId is used and we can't detect its value, disable the rule to avoid false positives.
-            shouldPerformUnusedCheck = false;
+            hasSeenUnknownMessageId = true;
           }
+
           values.forEach((val) => messageIdsUsed.add(val.value));
         }
       },
diff --git a/tests/lib/rules/no-unused-message-ids.js b/tests/lib/rules/no-unused-message-ids.js
index 0e92d49a..d35cadfe 100644
--- a/tests/lib/rules/no-unused-message-ids.js
+++ b/tests/lib/rules/no-unused-message-ids.js
@@ -220,6 +220,13 @@ ruleTester.run('no-unused-message-ids', rule, {
         }
       };
     `,
+    // Ignore when we couldn't find any calls to `context.report()`, likely because an external helper function is in use.
+    `
+      module.exports = {
+        meta: { messages: { foo: 'bar' } },
+        create(context) {}
+      };
+    `,
   ],
 
   invalid: [
@@ -313,29 +320,13 @@ ruleTester.run('no-unused-message-ids', rule, {
         },
       ],
     },
-    {
-      // messageId unused with no reports
-      code: `
-          module.exports = {
-            meta: { messages: { foo: 'hello world' } },
-            create(context) { }
-          };
-        `,
-      errors: [
-        {
-          messageId: 'unusedMessage',
-          data: { messageId: 'foo' },
-          type: 'Property',
-        },
-      ],
-    },
     {
       // messageId unused with meta.messages in variable
       code: `
           const messages = { foo: 'hello world' };
           module.exports = {
             meta: { messages },
-            create(context) { }
+            create(context) { context.report({node, messageId: 'other'}); }
           };
         `,
       errors: [
@@ -353,7 +344,7 @@ ruleTester.run('no-unused-message-ids', rule, {
           const extraMeta = { messages: { ...extraMessages } };
           module.exports = {
             meta: { ...extraMeta },
-            create(context) { }
+            create(context) { context.report({node, messageId: 'other'}); }
           };
         `,
       errors: [