react-hooks linter fails with unusual for loop

Created on 30 Nov 2018  路  6Comments  路  Source: facebook/react

Do you want to request a feature or report a bug?
bug
What is the current behavior?
React hooks linter fails with this piece of code

function useHook() {
  const ref = React.useRef([1, 2, 3]);
  const res = []
  // valid because there are not unexpected returns
  if (ref.current) {
    for (let i = 0; i !== ref.current.length && !ref.current[i]; ++i ) {
      res.push(ref.current[i]);
    }
  }
  React.useLayoutEffect(() => {});
}

however a bit more usual for loop is handled as expected

function useHook() {
  const ref = React.useRef([1, 2, 3]);
  const res = []
  if (ref.current) {
    for (let i = 0; i !== ref.current.length; ++i ) {
      res.push(ref.current[i]);
    }
  }
  React.useLayoutEffect(() => {});
}

What is the expected behavior?

Skip for looks without returns

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

eslint-plugin-react-hooks 0.0.0

challenging Bug good first issue

Most helpful comment

After some investigation, I've figured out a few things:

  1. First of all, the given code could be simplified:
    This code passes the linter
function Valid() {
  const res = [];
  for (let i = 0; i !== 10; ++i ) {
    res.push(i);
  }
  React.useLayoutEffect(() => {});
}

And this one doesn't:

function Bugged() {
  const res = [];
  const additionalCond = true;
  for (let i = 0; i !== 10 && additionalCond; ++i ) {
    res.push(i);
  }
  React.useLayoutEffect(() => {});
}
  1. Bug seems to appear when additional operand is added to for condition since ESLint forks the path during the analysis (the first operand can be either true or false and the same thing for second one). After ESLint code analysis we get following data structure which represents our code:
    eslint_diag

  2. Error seems to occur in countPathsFromStart function (source) and the main problem is caching. Commenting caching functionality out fixes the bug. For the above diagram, it says that there's one path from s2_6 to s2_1 while with disabled caching the result is two paths (that is the correct answer).

I guess that caching works wrong with cycles in the code path, but for now I have no idea why. Maybe some suggestions?
Also, can anyone clarify why are we always return 0 if a segment is contained by some cycle? (source line 125 and 131)

All 6 comments

I think it's a bug. Want to dig in?

I'll try

Code base seems not trivial. Can't figure out how to deal with code path segments.

After some investigation, I've figured out a few things:

  1. First of all, the given code could be simplified:
    This code passes the linter
function Valid() {
  const res = [];
  for (let i = 0; i !== 10; ++i ) {
    res.push(i);
  }
  React.useLayoutEffect(() => {});
}

And this one doesn't:

function Bugged() {
  const res = [];
  const additionalCond = true;
  for (let i = 0; i !== 10 && additionalCond; ++i ) {
    res.push(i);
  }
  React.useLayoutEffect(() => {});
}
  1. Bug seems to appear when additional operand is added to for condition since ESLint forks the path during the analysis (the first operand can be either true or false and the same thing for second one). After ESLint code analysis we get following data structure which represents our code:
    eslint_diag

  2. Error seems to occur in countPathsFromStart function (source) and the main problem is caching. Commenting caching functionality out fixes the bug. For the above diagram, it says that there's one path from s2_6 to s2_1 while with disabled caching the result is two paths (that is the correct answer).

I guess that caching works wrong with cycles in the code path, but for now I have no idea why. Maybe some suggestions?
Also, can anyone clarify why are we always return 0 if a segment is contained by some cycle? (source line 125 and 131)

I have successfully reproduced the symptom.
I gonna dig into this issue. Base on what @LosYear found, my plan is to figure out how the code path segments work and give a summary first, and result where the potential root cause is.
Gonna update if there is any progress. 馃憤

I've opened a PR with a possible solution on top of the awesome work @LosYear did. Thanks!

Was this page helpful?
0 / 5 - 0 ratings