WIP: react-hooks/exhaustive-deps

[alisnic]

I gotta be honest with you - I don't know Rust much and this may be too much for me. But I decided at least I can move things forwarding by creating the scaffolding and porting the tests

Related to #2174

[codspeed-hq]

CodSpeed Performance Report

Merging #2637 will not alter performance

_{Comparing alisnic:feat/exhaustive-deps (bdc8a38) with main (8e3e404)}

Summary

✅ 29 untouched benchmarks

[alisnic]

@Boshen pushed my latest code. Currently stuck by 2 things:

1. How do I check that the identifier id is defined within the component scope? So I don't generate false positives

function Counter() {
  let [count, setCount] = useState(0);

  function increment(x) {
    return x + 1;
  }

  useEffect(() => {
    let id = setInterval(() => {
      setCount(increment);
    }, 1000);
    return () => clearInterval(id);
  }, []);

  return <h1>{count}</h1>;
}

2. Given a AstNode, how to check that is it used inside of an expression that is assigned to itself?

function Example() {
  const foo = useCallback(() => {
    foo();
  }, []);
}

[alisnic]

I think I need a completely different approach here. Instead I should iterate over all identifier references and traverse the tree upwards to detect whether I'm in a hook or not. This should remove most of traversal boilerplate I wrote

[Boshen]

This rule is pretty complicated, let's see whether @rzvxa can help. Feel free to ask questions.

[rzvxa]

I think I need a completely different approach here. Instead I should iterate over all identifier references and traverse the tree upwards to detect whether I'm in a hook or not. This should remove most of traversal boilerplate I wrote

Hey mate, First off thanks for this impressive work.

I was reading through the code when I got an idea that might make it easier to implement, Have you tried to create a custom visitor instead of using the check_statements function?

If you add something like this:

#[derive(Default)]
struct DependencyCollector<'a> {
    deps: DependencyList<'a>,
}

impl<'a> Visit<'a> for DependencyCollector<'a> {
    fn visit_identifier_reference(&mut self, ident: &IdentifierReference<'a>) {
        self.deps.insert(/* dependency */);
    }
    /* .... */
}

and use it like this:

DependencyCollector::default().visit_statements(&body_expr.statements);

instead of calling the check_statement, It can abstract away most of the walk functions you've used and as a result make it less cluttered and easier to read/write.

I think it can result in what you want from visiting every identifier, I'm not sure which one is more performant though, Since one skips redundant checks but requires additional visits and the other one has a lot of redundant checks and no additional visit.

[rzvxa]

1. How do I check that the identifier id is defined within the component scope? So I don't generate false positives

function Counter() {
  let [count, setCount] = useState(0);

  function increment(x) {
    return x + 1;
  }

  useEffect(() => {
    let id = setInterval(() => {
      setCount(increment);
    }, 1000);
    return () => clearInterval(id);
  }, []);

  return <h1>{count}</h1>;
}

Given the scope_id of the id and the scope id for Counter, Can't we just check recursively whether this scope id has a direct ancestor which is the Counter?

But we have to check ScopeFlags and look for any shadowed variable in our path.

Maybe it can be easier if you go for checking the SymbolId instead of ScopeIds.

[alisnic]

I was reading through the code when I got an idea that might make it easier to implement, Have you tried to create a custom visitor instead of using the check_statements function?

How would this approach be different from:

    ctx.semantic().nodes().iter().for_each(|node| {
            let AstKind::IdentifierReference(iref) = node.kind() else { return };
            // do stuff
        })

Will the visitor visit nodes that the iter above would not?

[rzvxa]

I was reading through the code when I got an idea that might make it easier to implement, Have you tried to create a custom visitor instead of using the check_statements function?

How would this approach be different from:

    ctx.semantic().nodes().iter().for_each(|node| {
            let AstKind::IdentifierReference(iref) = node.kind() else { return };
            // do stuff
        })

Will the visitor visit nodes that the iter above would not?

I know there are some types missing from AstKind but to be honest I'm not sure if any of the vital ones are missing.

If there is nothing important missing in the AstKind then yeah your snippet above would basically do the same thing. But here is the thing, You didn't use the nodes iterator, Instead you rely on this recursive function which doesn't support all types containing an IdentifierReference - can't recall the exact type but I remember noticing one missing in my initial review - We are most probably going to use codegen for our visitors/AST since it is growing in complexity so using them would reduce the risk of human error.

Just to point out, We recently found out some types are missing a visit event in our Visit trait, So you can see even core stuff can be wrong when written by humans(since the visitor pattern is tedious and has a lot of boilerplate code).

But feel free to use either one of these approaches since they should perform the same.

[alisnic]

Hey guys, if anyone else wants to continue working on this please feel free. I thought I would be able to give it more time, but unfortunately I can't. My second child will be born soon so my attention it switched to other pressing matters 😄

[rzvxa]

Hey guys, if anyone else wants to continue working on this please feel free. I thought I would be able to give it more time, but unfortunately I can't. My second child will be born soon so my attention it switched to other pressing matters 😄

WOW, Congrats!

I'll pick it up in the future, And thanks for all the work you've done on this rule, I'll make sure you get credited as the co-author.

I'll announce when I start working on this issue so to anyone who wants to work on this, Until then feel free to propose a PR either based on this or written from scratch.

[overlookmotel]

@rzvxa I'm not clear how far off completion this is. If it's require a lot of work to finish off, and no obvious prospect of that happening in near future, shall we close it?

[rzvxa]

@rzvxa I'm not clear how far off completion this is. If it's require a lot of work to finish off, and no obvious prospect of that happening in near future, shall we close it?

I'm not sure about the correctness of it - as I haven't really reviewed it in depth - but it contains a tremendous amount of work, And all the tests(I assume).

It wouldn't hurt to keep it open, Or merge it without adding it to the rules(we already have some empty rules that have been merged after getting blocked, Some of the rules depending on control flow were amount these which since then I've implemented some of them). Either way we should make sure to credit the original author.

[alisnic]

Don't worry about credit guys, do what suits you best

[overlookmotel]

OK cool. I wasn't aware what a large feature this was. As you say, let's keep it open. I was only trying to do some "tidying up".

[tutturen]

The rule react-hooks/exhaustive-deps is the last missing piece for us to switch over to oxlint exclusively. Much needed!
I would like to help moving it forward, but I don't yet have any experience with oxc. And to be honest, it seems fairly complicated.

By the way, great work @alisnic and congrats with your second child!

[camc314]

going to take a look at this

[Boshen]

Continue in #7151