react-compiler transformed conditional useCallback call incorrectly

[huozhi]

Summary

Reproduction: https://github.com/beeebox/react-compiler-conditional-access-prop-repro

The reproduction is using react-compiler integration with Next.js

Steps to reproduce

1. Clone this repository
2. Run pnpm install
3. Run pnpm dev
4. Open http://localhost:3000 in your browser
5. You'll see the error in the dialog

Expected behavior

No error should be thrown

Actual behavior

Error is thrown

TypeError: Cannot read properties of null (reading 'name')

Root cause

The user object should be actually conditionally rendered since there's user ? condition to check to render the component when user is truthy. But react compiler still uses the user.name as condition for comparing with the memoized cache value. The check is hoisted in the render where the user can be null. This will trigger the error where we don't need to render the sub component where the callback is being used.

// Client output bundle for page.js
    _s();
    const $ = (0,react_compiler_runtime__WEBPACK_IMPORTED_MODULE_1__.c)(10);
    if ($[0] !== "5a27fdcf38c7f1295c3b71a91e1e520dce2311adaca1eb5d5515c0f76ddb4f25") {
        for(let $i = 0; $i < 10; $i += 1){
            $[$i] = Symbol.for("react.memo_cache_sentinel");
        }
        $[0] = "5a27fdcf38c7f1295c3b71a91e1e520dce2311adaca1eb5d5515c0f76ddb4f25";
    }
    const [user, setUser] = (0,react__WEBPACK_IMPORTED_MODULE_2__.useState)(null);
    let t0;
    if ($[1] !== user.name) {
        t0 = ()=>{
            console.log("Log user", user.name);
        };
        $[1] = user.name;
        $[2] = t0;
    } else {
        t0 = $[2];
    }
    const logUser = t0;

Potential Solution

Detect the condition of component rendering and conditionally update the callback?

[josephsavona]

Thanks for submitting. The compiler currently assumes that if a function accesses a value unconditionally, that it's safe to make that a dependency. Type systems like TypeScript and Flow will generally enforce that nullable/optional properties are checked before being accessed, which makes this work. So for example TS would have required a user?.name here.

However, we understand that not everyone is using TypeScript or Flow, so we are working to improve the inference in cases like this (part of why it's still experimental!).

For now, you can work around this by making the property access optional (user?.name) or by disabling the compiler in this component:

export default function Page() {
  "use no memo"; // <----- add this 
  const [user, setUser] = useState(null)
  ...

[josephsavona]

Playground repro is here: https://playground.react.dev/#N4Igzg9grgTgxgUxALhAgHgBwjALgAgBMEAzAQygBsCSoA7OXASwjvwAUyBzBACgEp8wADpt8cVmAIBtKGAQwANPnm4AqvJgBdfAF58chAGVcZXHzpVK-UaPzjJBShC4aFeg-IDCZSpQBGZHAA1rwCegB8Qnb2DnSQlAgAdM5cvADkADIungrpyoYwSXRkALYINmIAvsqymlqVtmIwCLiwbLwx9gA8hEwAbhFdsd0AFgCMEZw83QD0E0Nisfjd-lC4uKz4rF6UTCG6wOG6UapuMLzA+CXlyPjpAFIQo3Tp+FX8VRFGrbkwc2sNqxFst7MBCvgAPz4TpLUG9AYg0HLMYAJgi4M0xTKCCqc1G6OGyNW602bB2ewOwFS5y+2S40FwANJwKJI1mfUGbMEd0sfiqwzmnKRlSqICqQA

And with user?.name to guard the nullable access:
https://playground.react.dev/#N4Igzg9grgTgxgUxALhAgHgBwjALgAgBMEAzAQygBsCSoA7OXASwjvwAUyBzBACgEp8wADpt8cVmAIBtKGAQwANPnm4AqvJgBdfAF58chAGVcZXHzpVK-UaPzjJBShC4aFeg-IDCZSpQBGZHAA1rwCegB8Qnb2DnSQlAgAdM5cvADkADIungrpyoYwAPxJdGQAtgg2YgC+yrKaWtW2YjAIuLBsvDH2ADyETABuET2xvQAWAIwRnDy9APRTI2Kx+L3+ULi4rPisXpRMIbrA4bpRqm4wvMD4ZZXI+OkAUhDjdOn4Nfw1EUbtuTAFhstqxlqt7MBCvgivhuitwf0hmDwasJgAmCKQzSlCoIGoLcYY0Yo9abbZsPYHI7AVKXH7ZLjQXBAsmg4ljeYDYbswQPSx+GqjBZc5HVGogGpAA

[elsigh]

The example that spawned Jiachi's posting of this bug was generated from a project (my hobby thing) in strict mode typescript btw.

[huozhi]

Thanks for submitting. The compiler currently assumes that if a function accesses a value unconditionally, that it's safe to make that a dependency

Wonder if it could have some options to add nullable checks in the condition. This sounds a slightly eager on optimization which could break a lot of could where developers assume the values are not accessible in render.

[josephsavona]

Yeah, as I mentioned above the current output is safe if you’re following TS and ensuring you do bill checks. But we know that not everyone is using TS, so we are working on improved heuristics to add null checks ourselves where necessary.

[gnoff]

@josephsavona

Unfortunately TS is not very sound. (nor is flow in some cases like with array access) and so if you use the Record type you can get into a situation where TS thinks that unconditional access is safe but it actually isn't

https://www.typescriptlang.org/play/?strict=false&jsx=4#code/OQVwzgpgBAxgNgSwgOwC7AFAILYAcD2ATqlAN5TgQDCAhnHAEY0wDWANBZAMqo2rQBfKADNC+bFABEhCM1SSMGCAA8CxKABMIwmiDglhIZDFQJ8yKAAUaAcwgAKcpUJgOzgJIaoAgFxlOEC5+AEoQMEQaADxgqIQIyDYcpMg02BB+MXEJAgB8bpCEnhmx8TbeAJRkGFCw5jEBhFAAvA1gANoeGgC61VC94cj1cPg2AKoFzQG09Eys9vaVTTlVNTUDYPhwEAB0wzb2wAAyIw3A+YHbKWnlvQIcHQVdN4o1MqgghBb2vTWRGggANxyP1WkQAFgBGHLWOyRAD0kOBq1WpGcUAA-FBvsjkX9AUicbiwQAmHKogqXVIQATwkkEwm-BggVCocxQcxURCsJqkPbjQK5Y42fDM+FMlnmek4+H-IEgmqVPzIPRwAQgmX43o3ARAA

In this example users is a record and TS assumes the read will not produce undefined.

In the original motivating case the callback was only passed when the read did produce a value. So maybe in common cases you'd experience an error when unconditionally rendering some component. But once you "fix" that by gating the render on a read you are very unlikely to also gate the reads in the callback and in fact the callback is sort of valid in this case because it is only used when the value is not undefined.

So we're in a pickle where TS is unsound but the heuristic of the compiler shifts a read from being conditioned by something in the return slot to being unconditional.

IMO lifting the read into the body but not determining that the read is conditioned by something else like a boolean in the return slot means it'll never be truly safe to assume it is actually unconditional. Now if TS actually provided errors here I could see why we could lean on a type system as being the primary guard against this but since even our type systems don't flag this maybe the heuristic needs to soften.

Would it be terrible to treat useCallback reads as conditional by default since they are only executed outside the render and may be conditioned by something in the render?

[eps1lon]

As far as I can tell, this usage is sound. It's just that TypeScript (with noUncheckedIndexAccess) and Flow.js as well, don't realize that logUser is used in a scope where we closed over a refined user that is non-nullable.

It's now generally unsafe to hoist functions that are in a conditional branch out because the compiler doesn't preserve the control flow that lead to the usage of the function. But maybe that would be the solution here? Ensure that any condition that lead to function usage is also replayed when reading used values.

Type systems like TypeScript and Flow will generally enforce that nullable/optional properties are checked before being accessed, which makes this work.

TypeScript only does that for map-like types with noUncheckedIndexedAccess (which I see rarely used).

Flow.js doesn't do any index-access checking:

It is the programmer's responsibility to ensure the access is safe, as with arrays.

-- https://flow.org/en/docs/types/objects/#toc-objects-as-maps

You'd have to be explicit about it in Flow.js:

-users: { [string]: { name: string }}
+users: { [string]: { name: string } | undefined }

[gnoff]

Yeah what I meant by unsound is that reading from a Record can return undefined at runtime but the type system doesn't model this (except in TS if you use this option which I did not know about :) )

I didn't appreciate until now that the heuristic for useMemo, useEffect, and any other scopes that get created during a render is different than useCallback.

In all the other cases the inner scope is run unconditionally so the normal rules of assuming unconditional access being safe to transfer to the function scope is more or less fine b/c it'll error in render or it will error where used but you always get an error.

But useCallback is special because it might never be called. and if it is called it might be done only when some other condition is essentially checking/gating the reads in the inner scope. And this opens up the possibility for a valid React program to start to fail because now treating the apparent unconditional property access as safe to transfer to the function scope you break out of the control flow scope.

Basically unconditional property access might be transitively conditioned and no other React-ism really has this feature (that I can think of in the 5 minutes I spent writing this comment)

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[josephsavona]

We’re still working on this

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!