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.