[Compiler Bug]: Compiler incorrectly removes memoization of an expensive operation in useMemo

issacgerges commented 2 weeks ago

What kind of issue is this?

[X] React Compiler core (the JS output is incorrect, or your app works incorrectly after optimization)
[ ] babel-plugin-react-compiler (build issue installing or using the Babel plugin)
[ ] eslint-plugin-react-compiler (build issue installing or using the eslint plugin)
[ ] react-compiler-healthcheck (build issue installing or using the healthcheck script)

Link to repro

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAMygOzgFwJYSYAEUYCAsgJ4ASEEA1gBQCGAlEcADrFFyFg4iaOkQC8JMuQQBbCI0btRAPg7ci63v0GRpCHAAs8mAOZiiCAB4AHBJjB4AbghasA3Go0w9sYsyIBqIh09QxN3HgBfABoiAG1mAF1WDy8cHyE6cIiQCKA

Repro steps

The compiler seems to incorrectly erase memoization of possibly expensive operations using useMemo in some cases.

Compare the compiler output of the two similar examples, in the first all memoization will be replaced, in the 2nd it will be correctly added in

// all memoization is removed, incorrectly
function useMyHook(a) {
  const foo = useMemo(() => {
    const something = expensive(a);
    return a + something;
  }, [a])
  return foo;
}

// memoization is preserved/added, correctly
function useMyHook(a) {
  const foo = useMemo(() => {
    const something = expensive(a);
    return something;
  }, [a])
  return foo;
}

I believe this is a byproduct of how memoization is dependent on the return value.

Another side effect of that decision (although already a rule break) is that someone incorrectly using useMemo instead of useEffect would likely force that effect to run every render (regardless of their deps array that would normally prevent it)

How often does this bug happen?

Every time

What version of React are you using?

v17.0.2

poteto commented 2 weeks ago

Yeah the compiler currently does type inference to determine if something should be memoized or not. In this case, we don't memoize primitives as you show in your first example as the + operator makes the compiler assume that expensive(a) produces a primitive, which is always going to be referentially equal. I think the challenge here is that the compiler doesn't know that expensive() is expensive so if we apply memoization to primitive producing call expressions as well this would probably greatly increase the number of memo slots we use in the cache.

issacgerges commented 2 weeks ago

Ah, I see @poteto, thanks

I guess the thought is that memoization should be in expensive instead of react? This example also erases memorization, and the return value is a primitive but not necessarily cheap to produce.

(sorry this is sorta convoluted)

  const isNotPrime = useMemo(() => {
    const isPrime = checkIfPrime(a);
    return !isPrime
  }, [a])

I wonder if the compiler could/should "respect" the memorization provided if decides none is necessary. I'm sure these problems are more complicated than I'm giving them credit! Feel free to close if this is an intentional design decision

kaaboaye commented 1 week ago

Same issue as https://github.com/facebook/react/issues/29172 and https://github.com/facebook/react/issues/29580

facebook / react