[ESLint] Check useEvent references instead (#25319)

poteto · gaearon · web-flow · commit cb5084d1c4dc · 2022-09-24T01:34:17.000+01:00
* [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 &lt;dan.abramov@gmail.com&gt;

* Add comment

Co-authored-by: Dan Abramov &lt;dan.abramov@gmail.com&gt;
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());
         }
       },
     };
