Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Compiler Bug]: Compiler doesn't bail out when reading or writing ref.current during a render #29161

Closed
1 of 4 tasks
jthemphill opened this issue May 18, 2024 · 7 comments
Closed
1 of 4 tasks
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug

Comments

@jthemphill
Copy link

jthemphill commented May 18, 2024

What kind of issue is this?

  • 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/#N4Igzg9grgTgxgUxALhAegFQYDoDsAEG+AYlLnAC4CWEBAJhHAEYA2jA1snoft0diACaVBCzph8AQ3wwEAMwSzc1SSxYBPfGAqTWCfACV5+CEwBWCSvgoALSRXxUJqgO6T1YPvji0wTigjK+C5UttY2+gC2ENoyloEOAG6qUPoA5lSJgQB0Al5eAAowproaWjbQYrgA5A4+kQAOVCz6TJaSUGD6ofiRUDoBznFy+HSwVLhp4QheSnSK+MgAFPkERADKFVBi094Qjc0LcPZwNuFOAPx8aHhyZJQ0BJ0I6zp6RnJLySypAJT4wG4e1wsVkIwAvPhnh8vikEL8ANxAsHZOCwJQOSHfVJIghxCiwAhg3EAXzweEwODWJHu1Foo0YrA4XGp+RARgJMBBUmGigSVFUZQQAEcoKpvIKmJI4OwTAQEFkYJo5opcms2QBVXAsKjsfQAA2eAGFJdL2PqADS7Y5qKUyhkICS4CB1OyTfQuCK4LzzBqBebkEQSU6Sd25ECrHgAFRsTkcEmedEcBEioc0DRY0sdUjgxTAEmgMD28ylXWy+AAkhIemBJAovBQIFCutb9k0WkX6YXiwhS615BBZMEENUst4WqGEHQGxEW1AGlc1jdcHdyHSnl0TbazTCbSw7ex-oC8T4QQ4wfhIc9XqUELvTTLEeS8bJORuEFv92aVnj8PgltkgGSDAaRgP84IAHzDKi6IJABQEgWBFpAn+ADaKF-sMyG-vgAC62F-k+uAkiAJJAA

Repro steps

Our codebase has a core hook called useStableRef() which breaks one of the rules of React by mutating a ref during render. This code doesn't trigger the react-hooks lint rules, nor does the new React compiler complain when encountering this code.

I'm putting this forth in the hopes that the React compiler will bail out when it encounters one of these hooks, instead of compiling these hooks and potentially breaking our codebase when we upgrade.

  • useStableRef writes to ref.current in the hook's body. This is a side effect that occurs during rendering!
  • useCallbackRef calls into useStableRef to return a non-reactive callback. In practice, we're using this as a poor-company's useEffectEvent(), until that hook is stabilized. Should this function still compile if it calls a hook that doesn't compile?

How often does this bug happen?

Every time

What version of React are you using?

React Playground 2024-05-17

@jthemphill jthemphill added Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug labels May 18, 2024
@josephsavona
Copy link
Contributor

Thanks for posting. First note that refs are fine to access outside of render — in particular, in event handlers and effects. It's okay to access refs in a callback function, so long as that callback is only called outside of render.

So in your example, useStableRef() technically violates the rules. It's a KP that we're not catching it when ValidateNoRefAccessInRender is enabled, that's part of why that validation is off by default. useCallbackRef() technically doesn't violate the rules, but there's a huge caveat — it's only safe to call the resulting callbacks from outside of render.

Calling a callback created by this hook during render would mean that (just like accessing a ref) your UI could grow stale.

I'd give it a try and see.

josephsavona added a commit that referenced this issue May 20, 2024
Improves ValidateNoRefAccessInRender, detecting modifications of refs during render.

Fixes #29161

[ghstack-poisoned]
josephsavona added a commit that referenced this issue May 20, 2024
Improves ValidateNoRefAccessInRender, detecting modifications of refs during render.

Fixes #29161

ghstack-source-id: ef3b9a030be583c3a742135f31c998f9c57fd73e
Pull Request resolved: #29170
josephsavona added a commit that referenced this issue May 29, 2024
…ites of refs"

Improves ValidateNoRefAccessInRender, detecting modifications of refs during render.

Fixes #29161

[ghstack-poisoned]
josephsavona added a commit that referenced this issue May 29, 2024
Improves ValidateNoRefAccessInRender, detecting modifications of refs during render.

Fixes #29161

ghstack-source-id: 99078b3cea5b2d9019dbf77ede9c2e4cd9fbfd27
Pull Request resolved: #29170
josephsavona added a commit that referenced this issue May 29, 2024
Improves ValidateNoRefAccessInRender, detecting modifications of refs during render.

Fixes #29161

[ghstack-poisoned]
@jthemphill
Copy link
Author

🥳🥳🥳

@jthemphill
Copy link
Author

@josephsavona Thanks for handling this!

Our company is trying to figure out how to plan ahead for React 19, and whether we should prioritize removing useStableRef as shown in the playground example above.

When you say that useStableRef "technically violates the rules", do you anticipate it blocking compilation? Why is the validateRefAccessDuringRender option disabled by default, and do you think the default will change in the future?

@jthemphill
Copy link
Author

The other big question I have... there are hundreds of components which use useCallbackRef and hooks like it, which break the rules of React. Will/should all of them fail compilation?

According to npx react-compiler-healthcheck, it looks like the bad hooks themselves will fail compilation, but the components that use these hooks will still get compiled. But I don't understand why that should be the case, or if it should be correct to do so given that usually compilers start by inlining everything.

@josephsavona
Copy link
Contributor

When you say that useStableRef "technically violates the rules", do you anticipate it blocking compilation? Why is the validateRefAccessDuringRender option disabled by default, and do you think the default will change in the future?

Since I originally replied, we've improved this validation. It's now on by default and correctly flags the repro above as invalid.

whether we should prioritize removing useStableRef as shown in the playground example above.

As I noted above, the danger with this implementation is that your UI can grow stale. For example, the compiler may memoize a value based on the fact that the returned ref object never changes, even though it's internal value (.current) changes. However, if you're very careful to only access the value from outside of render then it may work for you. Note that you could still see weird behaviors; for example the ref may update during render, even if that render ends up suspending and getting cancelled. The more correct thing would be to wait to update the ref value until an effect.

@josephsavona
Copy link
Contributor

it looks like the bad hooks themselves will fail compilation, but the components that use these hooks will still get compiled. But I don't understand why that should be the case, or if it should be correct to do so given that usually compilers start by inlining everything.

Good question. Note that inlining is an optimization strategy - generally compilers (React Compiler included) implement static analysis without relying on inlining. Fundamentally, it's okay to violate the rules of React locally so long as the outer API does follow the rules. The examples here - useStableRef in particular - violate the rules in a way that is visible from the outside. Again, though, if you use them carefully then it can still be okay, but the onus is on you to be careful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug
Projects
None yet
Development

No branches or pull requests

2 participants