linter: react/rule-of-hooks false positives

[Boshen]

https://github.com/oxc-project/oxlint-ecosystem-ci/actions/runs/9061291729/job/24892841948?pr=2

I haven't checked the details, but I temporary moved the rule nursery 6edcae8 and is making a new release.

[alisnic]

I ran it in our product repo just now and it reported 232 false positives 😬. Will try to come up with a short repro when I have time. All the cases I saw were reporting

eslint-plugin-react-hooks(rules-of-hooks): React Hook "useEffect" may be executed more than once. Possibly because it is called in a loop. React Hooks must be called in the exact same order in every component render

The components do not have any loops. Edit: at least one comonent had a loop inside hook body

[alisnic]

export const FalsePositive = ({ editor, anchorElem, isLink, linkNodeUrl, close }: Props) => {
  // This custom hook invocation seems to trigger false positives below
  const [state, setState] = useCustomHook<State>({
    inputLinkUrl: linkNodeUrl ?? '',
    editable: !isLink,
    lastLinkUrl: '',
    lastSelection: null
  });

  const [someThing, setSomeThing] = useState(true);

  const onEdit = useCallback(() => setSomeThing(false), [inputLinkUrl, setSomeThing]);

  const updateLinkEditor = useCallback(() => {
    const rootElement = editor.getRootElement();

    if (nativeSelection.anchorNode === rootElement) {
      let inner = rootElement;
      while (inner.firstElementChild !== null) {
        inner = inner.firstElementChild as HTMLElement;
      }
    }
  }, [anchorElem, editor, setSomeThing]);

  return <div>test</div>;
};

  × eslint-plugin-react-hooks(rules-of-hooks): React Hook "useState" may be executed more than once. Possibly because it is called in a loop. React Hooks must be called in the exact same order in every component render.
    ╭─[imports/ui/client/components/compose/lexical/plugins/FloatingLinkPlugin/components/FloatingLinkEditor/FloatingLinkEditor.tsx:53:37]
 52 │
 53 │   const [someThing, setSomeThing] = useState(true);
    ·                                     ──────────────
 54 │
    ╰────

  × eslint-plugin-react-hooks(rules-of-hooks): React Hook "useCallback" may be executed more than once. Possibly because it is called in a loop. React Hooks must be called in the exact same order in every component render.
    ╭─[imports/ui/client/components/compose/lexical/plugins/FloatingLinkPlugin/components/FloatingLinkEditor/FloatingLinkEditor.tsx:55:18]
 54 │
 55 │   const onEdit = useCallback(() => setSomeThing, [inputLinkUrl, setSomeThing]);
    ·                  ─────────────────────────────────────────────────────────────
 56 │
    ╰────

  × eslint-plugin-react-hooks(rules-of-hooks): React Hook "useCallback" may be executed more than once. Possibly because it is called in a loop. React Hooks must be called in the exact same order in every component render.
    ╭─[imports/ui/client/components/compose/lexical/plugins/FloatingLinkPlugin/components/FloatingLinkEditor/FloatingLinkEditor.tsx:57:28]
 56 │
 57 │ ╭─▶   const updateLinkEditor = useCallback(() => {
 58 │ │       const rootElement = editor.getRootElement();
 59 │ │
 60 │ │       if (nativeSelection.anchorNode === rootElement) {
 61 │ │         let inner = rootElement;
 62 │ │         while (inner.firstElementChild !== null) {
 63 │ │           inner = inner.firstElementChild as HTMLElement;
 64 │ │         }
 65 │ │       }
 66 │ ╰─▶   }, [anchorElem, editor, setSomeThing]);
 67 │
    ╰────

[rzvxa]

export const FalsePositive = ({ editor, anchorElem, isLink, linkNodeUrl, close }: Props) => {
  // This custom hook invocation seems to trigger false positives below
  const [state, setState] = useCustomHook<State>({
    inputLinkUrl: linkNodeUrl ?? '',
    editable: !isLink,
    lastLinkUrl: '',
    lastSelection: null
  });

  const [someThing, setSomeThing] = useState(true);

  const onEdit = useCallback(() => setSomeThing(false), [inputLinkUrl, setSomeThing]);

  const updateLinkEditor = useCallback(() => {
    const rootElement = editor.getRootElement();

    if (nativeSelection.anchorNode === rootElement) {
      let inner = rootElement;
      while (inner.firstElementChild !== null) {
        inner = inner.firstElementChild as HTMLElement;
      }
    }
  }, [anchorElem, editor, setSomeThing]);

  return <div>test</div>;
};

  × eslint-plugin-react-hooks(rules-of-hooks): React Hook "useState" may be executed more than once. Possibly because it is called in a loop. React Hooks must be called in the exact same order in every component render.
    ╭─[imports/ui/client/components/compose/lexical/plugins/FloatingLinkPlugin/components/FloatingLinkEditor/FloatingLinkEditor.tsx:53:37]
 52 │
 53 │   const [someThing, setSomeThing] = useState(true);
    ·                                     ──────────────
 54 │
    ╰────

  × eslint-plugin-react-hooks(rules-of-hooks): React Hook "useCallback" may be executed more than once. Possibly because it is called in a loop. React Hooks must be called in the exact same order in every component render.
    ╭─[imports/ui/client/components/compose/lexical/plugins/FloatingLinkPlugin/components/FloatingLinkEditor/FloatingLinkEditor.tsx:55:18]
 54 │
 55 │   const onEdit = useCallback(() => setSomeThing, [inputLinkUrl, setSomeThing]);
    ·                  ─────────────────────────────────────────────────────────────
 56 │
    ╰────

  × eslint-plugin-react-hooks(rules-of-hooks): React Hook "useCallback" may be executed more than once. Possibly because it is called in a loop. React Hooks must be called in the exact same order in every component render.
    ╭─[imports/ui/client/components/compose/lexical/plugins/FloatingLinkPlugin/components/FloatingLinkEditor/FloatingLinkEditor.tsx:57:28]
 56 │
 57 │ ╭─▶   const updateLinkEditor = useCallback(() => {
 58 │ │       const rootElement = editor.getRootElement();
 59 │ │
 60 │ │       if (nativeSelection.anchorNode === rootElement) {
 61 │ │         let inner = rootElement;
 62 │ │         while (inner.firstElementChild !== null) {
 63 │ │           inner = inner.firstElementChild as HTMLElement;
 64 │ │         }
 65 │ │       }
 66 │ ╰─▶   }, [anchorElem, editor, setSomeThing]);
 67 │
    ╰────

Thanks for providing these examples, The ones without any loop inside of them are really weird to me. I check back edges for detecting cyclic flows and I already know some issues with the current cfg implementation that I'm going to fix while working on #3238. But this one shouldn't be one of them.

The last one is almost clear, I'm checking the child loops, and by mistake, I'm also walking in the NewFunction control flow edges. Maybe some of these are also related to this mistake of mine and result in an error in the wrong place.

None of the others seems to have a back edge at first glance, I have to investigate them.

If there is any other weird situation please make sure to dump them here. I can use them as test cases while working on an improved cfg and follow-up PR for this rule.

---

Edit:

I'm working on a fix that would resolve most of the issues with loop false positives, It would fix the errors mentioned above. I'll let you know when it is ready so you can rerun it on the project and report any other false positives there might be; Since there are probably some other cases that would confuse the linter.

[alisnic]

After upgrade to 0.4.1 all false positives are gone except 1 scenario. The rule does not seem to handle high order components correctly:

const Test = withCreatedModals(({ children, createdModals }: Props) => {
  const params = useParams<{ workspaceId: string; postId: string }>();

  return <h1>hello</h1>;
});

  × eslint-plugin-react-hooks(rules-of-hooks): React Hook "useParams" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function.
     ╭─[imports/ui/client/context/SplitLayoutContext/index.tsx:142:18]
 141 │ const Test = withCreatedModals(({ children, createdModals }: Props) => {
 142 │   const params = useParams<{ workspaceId: string; postId: string }>();
     ·                  ────────────────────────────────────────────────────
 143 │
     ╰────

[rzvxa]

After upgrade to 0.4.1 all false positives are gone except 1 scenario. The rule does not seem to handle high order components correctly:

const Test = withCreatedModals(({ children, createdModals }: Props) => {
  const params = useParams<{ workspaceId: string; postId: string }>();

  return <h1>hello</h1>;
});

  × eslint-plugin-react-hooks(rules-of-hooks): React Hook "useParams" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function.
     ╭─[imports/ui/client/context/SplitLayoutContext/index.tsx:142:18]
 141 │ const Test = withCreatedModals(({ children, createdModals }: Props) => {
 142 │   const params = useParams<{ workspaceId: string; postId: string }>();
     ·                  ────────────────────────────────────────────────────
 143 │
     ╰────

Well, those errors are intentional, But we may want to change it to a warning. I'm not sure how large of an impact this change is going to have, Tell me about your experience; How many false positives are out there in your project? and how many higher-order components do you guys have? do you think a whitelist for those function names can be more sufficient here?

Looking forward to your feedback.

[alisnic]

These false positives are not a problem for us at all. All errors come from a single file which was long due for a refactor anyway

[rzvxa]

Happy to hear that!