[ESLint] Check useEvent references instead (facebook#25319)

* [ESLint] Check useEvent references instead Previously the useEvent check in RulesOfHooks would collect all definitions of useEvent functions at the top level, record them as violations, then clear those violations if the useEvent function was later called or referened inside of an effect or another event. The flaw with this approach was in the special case where useEvent functions could be passed by reference inside of effects or events. The violation would be cleared here (since it was called at least once) and subsequent usages of the useEvent function would not be properly checked. This PR changes it so we check all identifiers that resolve to a useEvent function, and if they are not in an effect or event must be called or a lint error is emitted. Co-authored-by: Dan Abramov <[email protected]> * Add comment Co-authored-by: Dan Abramov <[email protected]>
misaeldossantos · Sep 24, 2022 · cb5084d · cb5084d
1 parent c89a836
commit cb5084d
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 59 deletions.
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
@@ -1090,7 +1090,7 @@ if (__EXPERIMENTAL__) {
         return <Child onClick={() => onClick()} />
       }
     `,
-      errors: [{...useEventError('onClick'), line: 4}],
+      errors: [{...useEventError('onClick'), line: 7}],
     },
     {
       code: `
@@ -1110,13 +1110,26 @@ if (__EXPERIMENTAL__) {
         const onClick = useEvent(() => {
           showNotification(theme);
         });
-        let foo;
-        useEffect(() => {
-          foo = onClick;
-        });
+        let foo = onClick;
         return <Bar onClick={foo} />
       }
     `,
+      errors: [{...useEventError('onClick'), line: 7}],
+    },
+    {
+      code: `
+        // Should error because it's being passed down to JSX, although it's been referenced once
+        // in an effect
+        function MyComponent({ theme }) {
+          const onClick = useEvent(() => {
+            showNotification(them);
+          });
+          useEffect(() => {
+            setTimeout(onClick, 100);
+          });
+          return <Child onClick={onClick} />
+        }
+      `,
       errors: [useEventError('onClick')],
     },
   ];

diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
@@ -120,42 +120,30 @@ export default {
     let lastEffect = null;
     const codePathReactHooksMapStack = [];
     const codePathSegmentStack = [];
-    const useEventViolations = new Set();
-
-    // For a given AST node, iterate through the top level statements and add all useEvent
-    // definitions. We can do this in non-Program nodes because we can rely on the assumption that
-    // useEvent functions can only be declared within a component or hook at its top level.
-    function addAllUseEventViolations(node) {
-      if (node.body.type !== 'BlockStatement') return;
-      for (const statement of node.body.body) {
-        if (statement.type !== 'VariableDeclaration') continue;
-        for (const declaration of statement.declarations) {
-          if (
-            declaration.type === 'VariableDeclarator' &&
-            declaration.init &&
-            declaration.init.type === 'CallExpression' &&
-            declaration.init.callee &&
-            isUseEventIdentifier(declaration.init.callee)
-          ) {
-            useEventViolations.add(declaration.id);
+    const useEventFunctions = new WeakSet();
+
+    // For a given scope, iterate through the references and add all useEvent definitions. We can
+    // do this in non-Program nodes because we can rely on the assumption that useEvent functions
+    // can only be declared within a component or hook at its top level.
+    function recordAllUseEventFunctions(scope) {
+      for (const reference of scope.references) {
+        const parent = reference.identifier.parent;
+        if (
+          parent.type === 'VariableDeclarator' &&
+          parent.init &&
+          parent.init.type === 'CallExpression' &&
+          parent.init.callee &&
+          isUseEventIdentifier(parent.init.callee)
+        ) {
+          for (const ref of reference.resolved.references) {
+            if (ref !== reference) {
+              useEventFunctions.add(ref.identifier);
+            }
           }
         }
       }
     }
 
-    // Resolve a useEvent violation, ie the useEvent created function was called.
-    function resolveUseEventViolation(scope, ident) {
-      if (scope.references == null || useEventViolations.size === 0) return;
-      for (const ref of scope.references) {
-        if (ref.resolved == null) continue;
-        const [useEventFunctionIdentifier] = ref.resolved.identifiers;
-        if (ident.name === useEventFunctionIdentifier.name) {
-          useEventViolations.delete(useEventFunctionIdentifier);
-          break;
-        }
-      }
-    }
-
     return {
       // Maintain code segment path stack as we traverse.
       onCodePathSegmentStart: segment => codePathSegmentStack.push(segment),
@@ -567,11 +555,6 @@ export default {
           reactHooks.push(node.callee);
         }
 
-        const scope = context.getScope();
-        // useEvent: Resolve a function created with useEvent that is invoked locally at least once.
-        // OK - onClick();
-        resolveUseEventViolation(scope, node.callee);
-
         // useEvent: useEvent functions can be passed by reference within useEffect as well as in
         // another useEvent
         if (
@@ -587,9 +570,21 @@ export default {
       },
 
       Identifier(node) {
-        // OK - useEffect(() => { setInterval(onClick, ...) }, []);
-        if (lastEffect != null && node.parent.type === 'CallExpression') {
-          resolveUseEventViolation(context.getScope(), node);
+        // This identifier resolves to a useEvent function, but isn't being referenced in an
+        // effect or another event function. It isn't being called either.
+        if (
+          lastEffect == null &&
+          useEventFunctions.has(node) &&
+          node.parent.type !== 'CallExpression'
+        ) {
+          context.report({
+            node,
+            message:
+              `\`${context.getSource(
+                node,
+              )}\` is a function created with React Hook "useEvent", and can only be called from ` +
+              'the same component. They cannot be assigned to variables or passed down.',
+          });
         }
       },
 
@@ -602,27 +597,14 @@ export default {
       FunctionDeclaration(node) {
         // function MyComponent() { const onClick = useEvent(...) }
         if (isInsideComponentOrHook(node)) {
-          addAllUseEventViolations(node);
+          recordAllUseEventFunctions(context.getScope());
         }
       },
 
       ArrowFunctionExpression(node) {
         // const MyComponent = () => { const onClick = useEvent(...) }
         if (isInsideComponentOrHook(node)) {
-          addAllUseEventViolations(node);
-        }
-      },
-
-      'Program:exit'(_node) {
-        for (const node of useEventViolations.values()) {
-          context.report({
-            node,
-            message:
-              `\`${context.getSource(
-                node,
-              )}\` is a function created with React Hook "useEvent", and can only be called from ` +
-              'the same component. They cannot be assigned to variables or passed down.',
-          });
+          recordAllUseEventFunctions(context.getScope());
         }
       },
     };