[compiler] Fix false positive memo validation

[josephsavona]

Partial fix for #34262. Consider this example:

function useInputValue(input) {
  const object = React.useMemo(() => {
    const {value} = transform(input);
    return {value};
  }, [input]);
  return object;
}

React Compiler breaks this code into two reactive scopes:

• One for transform(input)
• One for {value}

When we run ValidatePreserveExistingMemo, we see that the scope for {value} has the dependency value, whereas the original memoization had the dependency input, and throw an error that the dependencies didn't match.

In other words, we're flagging the fact that memoized better than the user as a problem. The more complete solution would be to validate that there is a subgraph of reactive scopes with a single input and output node, where the input node has the same dependencies as the original useMemo, and the output has the same outputs. That is true in this case, with the subgraph being the two consecutive scopes mentioned above.

But that's complicated. As a shortcut, this PR checks for any dependencies that are defined after the start of the original useMemo. If we find one, we know that it's a case where we were able to memoize more precisely than the original, and we don't report an error on the dependency. We still check that the original output value is able to be memoized, though. So if the scope of object were extended, eg with a call to mutate(object), then we'd still correctly report an error that we couldn't preserve memoization.

---

Stack created with Sapling. Best reviewed with ReviewStack.

• -> [compiler] Fix false positive memo validation #34276
• [compiler] Handle empty list of eslint suppression rules #34323

[josephsavona]

The isUnmemoized() check here is for sanity. I can't construct an example where it wouldn't be memoized, since the dep in this case would be constructed within the useMemo block. The only way to force such a dep to not memoize is if it somehow mixes up with a scope from outside the memo block, in which it would take on a dep that is mutated later which fails a separate validation.

[josephsavona]

added a fixture demonstrating that case just to be safe

[poteto]

I defer to @mofeiZ to review the actual fix, but the fixtures make sense to me

[poteto]

should this comment be updated, since it's now fixed?

[mofeiZ]

Interesting, from a first read it feels like declsWithinMemoBlock is not tracking the correct set of declarations in this case. Will do a local checkout and try to understand the difference

[josephsavona]

Interesting, from a first read it feels like declsWithinMemoBlock is not tracking the correct set of declarations in this case. Will do a local checkout and try to understand the difference

I don't think that's it. In the example from the description, the declarations set is correctly set to the {value} temporary.

[mofeiZ]

#34319 here is an alternative implementation that uses declsWithinMemoBlock