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 · 3 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

🥳🥳🥳

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