refactor: get upstream references instead of variables

better low-level API - we can still do our variable stuff via
`ref.resolved`, but now have the refs available to analyze too

Author: NickvanDyke

src/no-derived-state.js

@@ -5,10 +5,10 @@ import {
   isStateSetter,
   getUseStateNode,
   isProp,
-  getUpstreamReactVariables,
   isState,
   hasCleanup,
   isUseEffect,
+  getUpstreamRefs,
 } from "./util/react.js";
 import { getCallExpr, getDownstreamRefs } from "./util/ast.js";
 
@@ -52,14 +52,16 @@ export default {
             (ref) => isState(context, ref) || isProp(context, ref),
           );
 
-          const argsUpstreamVars = argsRefs.flatMap((ref) =>
-            getUpstreamReactVariables(context, ref.resolved),
+          const argsUpstreamRefs = argsRefs.flatMap((ref) =>
+            getUpstreamRefs(context, ref),
           );
-          const depsUpstreamVars = depsRefs.flatMap((ref) =>
-            getUpstreamReactVariables(context, ref.resolved),
+          const depsUpstreamRefs = depsRefs.flatMap((ref) =>
+            getUpstreamRefs(context, ref),
           );
-          const isAllArgsInDeps = argsUpstreamVars.notEmptyEvery((argVar) =>
-            depsUpstreamVars.some((depVar) => argVar.name === depVar.name),
+          const isAllArgsInDeps = argsUpstreamRefs.notEmptyEvery((argRef) =>
+            depsUpstreamRefs.some(
+              (depRef) => argRef.resolved.name === depRef.resolved.name,
+            ),
           );
           const isValueAlwaysInSync = isAllArgsInDeps && countCalls(ref) === 1;
 

src/util/ast.js

@@ -1,5 +1,11 @@
 import { findVariable } from "eslint-utils";
 
+/**
+ * @import {Scope} from 'eslint'
+ * @import {Rule} from 'eslint'
+ * @import {AST} from 'eslint'
+ */
+
 export const traverse = (context, node, visit, visited = new Set()) => {
   if (visited.has(node)) {
     return;
@@ -31,42 +37,10 @@ export const findDownstreamNodes = (context, topNode, type) => {
   return nodes;
 };
 
-export const getUpstreamVariables = (
-  context,
-  variable,
-  filter,
-  visited = new Set(),
-) => {
-  if (visited.has(variable)) {
-    return [];
-  }
-
-  visited.add(variable);
-
-  const upstreamVariables = variable.defs
-    // TODO: https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/34
-    // `init` covers for arrow functions; also needs `body` to descend into function declarations
-    // But then for function parameters (including props), `def.node.body` is the body of the function that they belong to,
-    // so we get *all* the downstream refs in it...
-    // We only want to descend when we're traversing up the function itself; no its parameters.
-    // Probably similar logic to in `getUpstreamReactVariables`.
-    .filter((def) => !!def.node.init)
-    .filter((def) => filter(def.node))
-    .flatMap((def) => getDownstreamRefs(context, def.node.init))
-    .map((ref) => ref.resolved)
-    // I think this only happens when:
-    // 1. There's genuinely no variable, i.e. `node` is a literal
-    // 2. Import statement is missing
-    // 3. ESLint globals are misconfigured
-    .filter(Boolean)
-    .flatMap((variable) =>
-      getUpstreamVariables(context, variable, filter, visited),
-    );
-
-  // Ultimately return only leaf variables
-  return upstreamVariables.length === 0 ? [variable] : upstreamVariables;
-};
-
+/**
+ * @param {Rule.RuleContext} context
+ * @param {Rule.Node} node
+ */
 export const getDownstreamRefs = (context, node) =>
   findDownstreamNodes(context, node, "Identifier")
     .map((identifier) => getRef(context, identifier))

src/util/react.js

@@ -1,4 +1,10 @@
-import { getUpstreamVariables, getDownstreamRefs, getCallExpr } from "./ast.js";
+import { getDownstreamRefs, getCallExpr } from "./ast.js";
+
+/**
+ * @import {Scope} from 'eslint'
+ * @import {Rule} from 'eslint'
+ * @import {AST} from 'eslint'
+ */
 
 export const isReactFunctionalComponent = (node) =>
   (node.type === "FunctionDeclaration" ||
@@ -130,23 +136,24 @@ export const isPropCallback = (context, ref) =>
 // NOTE: Global variables (like `JSON` in `JSON.stringify()`) have an empty `defs`; fortunately `[].some() === false`.
 // Also, I'm not sure so far when `defs.length > 1`... haven't seen it with shadowed variables or even redeclared variables with `var`.
 export const isState = (context, ref) =>
-  getUpstreamReactVariables(context, ref.resolved).notEmptyEvery((variable) =>
-    variable.defs.some((def) => isUseState(def.node)),
+  getUpstreamRefs(context, ref).notEmptyEvery((ref) =>
+    ref.resolved.defs.some((def) => isUseState(def.node)),
   );
 // Returns false for props of HOCs like `withRouter` because they usually have side effects.
 export const isProp = (context, ref) =>
-  getUpstreamReactVariables(context, ref.resolved).notEmptyEvery((variable) =>
-    variable.defs.some((def) => isPropDef(def)),
+  getUpstreamRefs(context, ref).notEmptyEvery((ref) =>
+    ref.resolved.defs.some((def) => isPropDef(def)),
   );
 export const isRef = (context, ref) =>
-  getUpstreamReactVariables(context, ref.resolved).notEmptyEvery((variable) =>
-    variable.defs.some((def) => isUseRef(def.node)),
+  getUpstreamRefs(context, ref).notEmptyEvery((ref) =>
+    ref.resolved.defs.some((def) => isUseRef(def.node)),
   );
 
 // TODO: Surely can be simplified/re-use other functions.
 // Needs a better API too so we can more easily get names etc. for messages.
 export const getUseStateNode = (context, ref) => {
-  return getUpstreamReactVariables(context, ref.resolved)
+  return getUpstreamRefs(context, ref)
+    .map((ref) => ref.resolved)
     .find((variable) => variable.defs.some((def) => isUseState(def.node)))
     ?.defs.find((def) => isUseState(def.node))?.node;
 };
@@ -186,25 +193,43 @@ export const isImmediateCall = (node) => {
 export const isArgsAllLiterals = (context, callExpr) =>
   callExpr.arguments
     .flatMap((arg) => getDownstreamRefs(context, arg))
-    .flatMap((ref) => getUpstreamReactVariables(context, ref.resolved))
-    .length === 0;
-
-export const getUpstreamReactVariables = (context, variable) =>
-  getUpstreamVariables(
-    context,
-    variable,
-    // Stop at the *usage* of `useState` - don't go up to the `useState` variable.
-    // Not needed for props - they don't go "too far".
-    // We could remove this and check for the `useState` variable instead,
-    // but then all our tests need to import it so we can traverse up to it.
-    // And would need to change `getUseStateNode()` too?
-    // TODO: Could probably organize these filters better.
-    (node) => !isUseState(node),
-  ).filter((variable) =>
-    variable.defs.every(
-      (def) =>
-        isPropDef(def) ||
-        // Ignore variables declared inside an anonymous function, like in `array.map()`.
-        def.type !== "Parameter",
-    ),
-  );
+    .flatMap((ref) => getUpstreamRefs(context, ref)).length === 0;
+
+/**
+ * @param {Rule.RuleContext} context
+ * @param {Scope.Reference} ref
+ *
+ * @returns {Scope.Reference[]}
+ */
+export const getUpstreamRefs = (context, ref) => {
+  if (!ref.resolved) {
+    // I think this only happens when:
+    // 1. Import statement is missing
+    // 2. ESLint globals are misconfigured
+    return [];
+  } else if (
+    // Ignore references to function parameters, aside from props.
+    ref.resolved.defs.every(
+      (def) => def.type === "Parameter" && !isPropDef(def),
+    )
+  ) {
+    return [];
+  }
+
+  const upstreamRefs = ref.resolved.defs
+    // TODO: https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/34
+    // `init` covers for arrow functions; also needs `body` to descend into function declarations
+    // But then for function parameters (including props), `def.node.body` is the body of the function that they belong to,
+    // so we get *all* the downstream refs in it...
+    // We only want to descend when we're traversing up the function itself; no its parameters.
+    // Probably similar logic to in `getUpstreamReactVariables`.
+    .filter((def) => !!def.node.init)
+    // Stop before we get to `useState()` - we want references to the state and setter.
+    // May not be necessary if we adapt the check in `isState()`?
+    .filter((def) => !isUseState(def.node))
+    .flatMap((def) => getDownstreamRefs(context, def.node.init))
+    .flatMap((ref) => getUpstreamRefs(context, ref));
+
+  // Ultimately return only leaf refs
+  return upstreamRefs.length === 0 ? [ref] : upstreamRefs;
+};

test/no-derived-state.test.js

@@ -207,6 +207,11 @@ new MyRuleTester().run("no-derived-state", rule, {
           useEffect(() => {
             setCountJson(JSON.stringify(count));
           }, [count]);
+
+          return (
+            // So single-setter doesn't trigger
+            <button onClick={() => setCountJson(undefined)}>reset</button>
+          )
         }
       `,
     },
@@ -217,9 +222,13 @@ new MyRuleTester().run("no-derived-state", rule, {
           const [multipliedCount, setMultipliedCount] = useState();
 
           useEffect(() => {
-            const multipler = fetch('/multipler');
-            setMultipliedCount(count * multipler);
+            setMultipliedCount(count * fetch('/multipler'));
           }, [count]);
+
+          return (
+            // So single-setter doesn't trigger
+            <button onClick={() => setMultipliedCount(0)}>reset</button>
+          )
         }
       `,
     },
@@ -826,7 +835,7 @@ new MyRuleTester().run("no-derived-state", rule, {
 
           useEffect(() => {
             setFullName(computeName(firstName, lastName));
-          }, [firstName, lastName]);
+          }, [firstName, lastName, computeName]);
         }
       `,
       errors: [

test/syntax.test.js

@@ -479,18 +479,17 @@ new MyRuleTester().run("syntax", noDerivedState, {
       code: js`
         import { useState, useEffect } from 'react';
 
-        function CountAccumulator({ count }) {
-          const [total, setTotal] = useState(count);
+         function DoubleCounter() {
+           const [count, setCount] = useState(0);
+           const [doubleCount, setDoubleCount] = useState(0);
 
-          useEffect(() => {
-            setTotal((prev) => prev + count);
-          }, [count]);
-        }
+           useEffect(() => setDoubleCount(count * 2), [count]);
+         }
       `,
       errors: [
         {
           messageId: "avoidDerivedState",
-          data: { state: "total" },
+          data: { state: "doubleCount" },
         },
       ],
     },